Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving scripts to within python source code folder #3297

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

shimwell
Copy link
Member

@shimwell shimwell commented Feb 12, 2025

Description

Moves the scripts folder to within the python package

@tkittel is this the change you had in mind for making the scripts a little more standard and more compatible with windows

Fixes # (issue)
helps with future windows compatibility

Checklist

  • I have performed a self-review of my own code
  • I have followed the style guidelines for Python source files (if applicable)

@tkittel
Copy link
Contributor

tkittel commented Feb 12, 2025

Almost, but I think you need to go through each script file itself and also check that it actually defines a function called main. For instance I looked in the ace-to-hdf5 file and it simply ends with a:

if __name__ == '__main__':
     #lots of code here

Whereas it should instead follow the format:

def main():
     #lots of code here

if __name__ == '__main__':
     main()

(and of course you can in principle do without the final two lines as well, but it might be convenient to keep them in for obscure usecases).

@tkittel
Copy link
Contributor

tkittel commented Feb 12, 2025

And I should say that I don't have much experience with these nested submodules you are using in openmc, so I never actually tried to point at those in projects.scripts

@tkittel
Copy link
Contributor

tkittel commented Feb 12, 2025

Oh, and you are also missing a .py extension on the moved files (they really are no longer scripts, they are regular python modules that you could import).

If it doesn't work, you might have to rename them. For instance instead of:

openmc/scripts/openmc-ace-to-hdf5

This for sure will work:

openmc/_scripts_openmc-ace-to-hdf5.py

with:

[project.scripts]
openmc-ace-to-hdf5 = "openmc._scripts_openmc_ace_to_hdf5:main"

@tkittel
Copy link
Contributor

tkittel commented Feb 12, 2025

Another advantage of having all of the scripts as python modules is that they can simply do relative imports, i.e. instead of:

import openmc.foo as bar

they can write:

import .foo as bar

Which is nice, as it for instance prevents the mistake I did and the "import openmc" actually finds the openmc folder in my current working directory instead of the one in python site-packages :-)

@shimwell
Copy link
Member Author

I've tinkered a bit more as the scripts were failing when called in the terminal.

Looks like the advice is to add main functions with None as the default args then call the arg parser if the arguments are none

To be honest I think all of these scripts are redundant and we can do all the same functionality with the python API so I don't think they are necessary.

However this PR is an option if we want to bring them up to date with packaging recommendations and windows compatibility. If we want to keep these scripts we might as well have them in the openmc/scripts folder instead of their current location scripts folder which makes packaging and windows compatibility harder

Note the openmc-plot-mesh-tally script is failing with a matplot lib error as it looks like that script was untested and matplot lib has moved on.

    matplotlib.use("TkAgg")
    ^^^^^^^^^^^^^^
AttributeError: module 'matplotlib' has no attribute 'use'

I can put in another PR that simple deletes the scripts if that is preferable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants