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

YAML reader function #70

Merged
merged 10 commits into from
Apr 25, 2024
Merged

YAML reader function #70

merged 10 commits into from
Apr 25, 2024

Conversation

connoramoreno
Copy link
Collaborator

@connoramoreno connoramoreno commented Mar 30, 2024

Changes the location of the YAML reader function read_yaml_config. Previously, each ParaStell script had an identical read_yaml_config function to read YAML input. This PR moves this function to utils.py, and each script then imports it.

Closes #52
Closes #54

@connoramoreno
Copy link
Collaborator Author

It should be noted that this is a draft since the relative imports in each script seem to be causing issues when using YAML input from the command line. For example, assuming parastell.py and config.yaml are in the same directory, the command

python3 parastell.py config.yaml

results in the following error:

Traceback (most recent call last):
  File "/home/camoreno/parastell/parastell/parastell.py", line 6, in <module>
    from . import log
ImportError: attempted relative import with no known parent package

@gonuke
Copy link
Member

gonuke commented Mar 31, 2024

I think we'll want to get to a point where we actually install this rather than just run from the source directory. Alternatively, we put the parastell directory in the python path?

@connoramoreno
Copy link
Collaborator Author

This is with parastell on PYTHONPATH, specifically the repository directory. This also uses the updated file structure.

@connoramoreno
Copy link
Collaborator Author

Executing from a different directory with config.yaml,

python3 ~/parastell/parastell/parastell.py config.yaml

results in the same error.

@connoramoreno
Copy link
Collaborator Author

I can change it so that we don't use relative imports; it's just strange that they seem to cause issues

@gonuke
Copy link
Member

gonuke commented Apr 2, 2024

If that fixes it, I guess let's change it. Separately, I'd like to better understand these relative imports, because they should work.

@connoramoreno
Copy link
Collaborator Author

connoramoreno commented Apr 2, 2024

The relative imports work when ParaStell is imported as a module, but not when run from command line with a YAML input argument.

@connoramoreno
Copy link
Collaborator Author

It might be that relative imports are defined only for Python modules, but when these scripts are run from command line with a YAML argument, they might not be considered modules by Python anymore

@gonuke
Copy link
Member

gonuke commented Apr 3, 2024

Yes... so I think the right way to do this is to generate a lightweight script that imports parastell and calls parastell.parastell()

if you make a file called stellarator with these contents and chmod u+x, it works.

#!/usr/bin/env python3

from parastell import parastell

parastell.parastell()

and make it executable.

I think there are simple ways to do this with setup tools as part of installation. But that may be for another time.

@connoramoreno connoramoreno linked an issue Apr 3, 2024 that may be closed by this pull request
@connoramoreno connoramoreno self-assigned this Apr 9, 2024
@connoramoreno connoramoreno requested a review from gonuke April 9, 2024 15:44
@connoramoreno connoramoreno marked this pull request as ready for review April 17, 2024 13:59
@connoramoreno
Copy link
Collaborator Author

I am planning to lump in the solution we discussed for #52 into this PR since it's so related, unless there are any objections.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor comment on naming the executables.

executables/invessel_build.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if we figure out how to use pip install correctly, these can be automatically generated in the right place in the path.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we rather make ParaStell pip-installable or condo-installable, given the dependency on conda packages? I'm not sure that some of our dependencies can be installed via pip.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pip can also be used to install the software from the source code in a local place on the Python path. This is the mode I'm thinking of, not necessarily installing with pip from PyPi

@connoramoreno connoramoreno linked an issue Apr 24, 2024 that may be closed by this pull request
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks @connoramoreno

@gonuke gonuke merged commit c0488f9 into oo_version Apr 25, 2024
@connoramoreno connoramoreno deleted the yaml_input branch April 26, 2024 20:14
connoramoreno pushed a commit that referenced this pull request Sep 10, 2024
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.

Standardize code patterns for reading input files Review structure of YAML file
2 participants