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

Check pyiron installation #1131

Closed
wants to merge 3 commits into from
Closed

Check pyiron installation #1131

wants to merge 3 commits into from

Conversation

jan-janssen
Copy link
Member

Current checks implemented:

  • executable bit is set for shell scripts
  • conda package is installed

These can be executed using:

import pyiron_base
pyiron_base.check_executables_status()

Which returns a pandas dataframe with all executables pyiron found.

Current checks implemented:
- executable bit is set for shell scripts
- conda package is installed

These can be executed using:
```
import pyiron_base
pyiron_base.check_executables_status()
```
Which returns a pandas dataframe with all executables pyiron found.
@jan-janssen jan-janssen added the format_black reformat the code using the black standard label Jun 13, 2023
@pmrv
Copy link
Contributor

pmrv commented Jun 14, 2023

Would be nice to have this also available via python -m pyiron_base.cli, but lgtm otherwise.

Copy link
Member

@liamhuber liamhuber left a comment

Choose a reason for hiding this comment

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

This still needs a test. We can explicitly add some dummy scripts to the static directory. Whenever there is path based stuff, it's much safe to have it appear in the CI so we don't accidentally specify something in a windows non-compliant way.


path_lst = []
for res_path in resource_paths:
path_lst += glob.glob(res_path + "/*/*/*.sh")
Copy link
Member

Choose a reason for hiding this comment

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

This feels very fragile. It looks to me like this requires all executables to be at exactly the third level of depth. This may be the case right now, but so we specify or document this anywhere? Do we anticipate other sh files appearing here, or is it safe to run an endswith command recursively inside the resource path instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right now it needs to be exactly at the third level because the expected structure is /$resources/$module_name/bin/$exe.sh. The documentation for this is here, though it could be more explicit.

I have a little class that codifies this in tinybase which could be moved to base. Replacing this everywhere might be a bit more effort, so imo it's ok to do it in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

I'm ok with codefying it as long as we're sufficiently explicit. One option I see is to search for scripts at exactly three levels deep and also searching for them anywhere in the resource tree, then giving the user a warning if the lists aren't the same.

Actually, I think I really like that, and it should be straightforward to make a test setup in static to make sure both aspects work.

Similarly we should also be checking if non-supported codes have directories. I'm on mobile so I can't easily bail on this comment to check if that's already done, but this would be another nice thing to give a warning about.

@stale
Copy link

stale bot commented Aug 12, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 12, 2023
@jan-janssen jan-janssen removed the format_black reformat the code using the black standard label Aug 14, 2023
@stale stale bot removed the stale label Aug 14, 2023
@jan-janssen jan-janssen added stale format_black reformat the code using the black standard labels Aug 14, 2023
@stale stale bot removed the stale label Aug 14, 2023
@jan-janssen jan-janssen added the help wanted Extra attention is needed label Aug 14, 2023
@stale
Copy link

stale bot commented Sep 17, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 17, 2023
@jan-janssen jan-janssen marked this pull request as draft February 14, 2024 12:26
@stale stale bot removed the stale label Feb 14, 2024
@jan-janssen jan-janssen deleted the check_executables branch February 19, 2024 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
format_black reformat the code using the black standard help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants