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

Flux cleanup #585

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Flux cleanup #585

merged 5 commits into from
Jan 31, 2024

Conversation

jameshcorbett
Copy link
Collaborator

Cleaning up some of the Flux code.

Problem: the FLUX ResourceManager docstrings are a bit messy.

Clean them up and make them PEP8 compliant.
Problem: the Flux ResourceManager subclass in unnecessarily
complicated.

Clean it up and simplify the logic for checking down nodes.
Problem: 'bare' try/excepts catch all manner of exceptions and
sometimes hide the true source of error.

Add better handling for importing and using flux.
Problem: the Flux JobLauncher subclass has some stray variables and
unidiomatic Python code.

Clean it up.
Problem: the Flux RemoteExec subclass has some duplicated and messy
code.

Clean it up.
@jameshcorbett jameshcorbett marked this pull request as ready for review January 31, 2024 17:48
Copy link
Collaborator

@mcfadden8 mcfadden8 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! I just had a couple of questions inline. I'm new here, but do we have CI that tests this? I imagine this may be difficult or not possible?

scripts/python/scrjob/launchers/flux.py Show resolved Hide resolved
scripts/python/scrjob/launchers/flux.py Show resolved Hide resolved
@jameshcorbett
Copy link
Collaborator Author

Looks great, thanks! I just had a couple of questions inline. I'm new here, but do we have CI that tests this? I imagine this may be difficult or not possible?

We should definitely get CI that tests this if we don't have it!

@mcfadden8
Copy link
Collaborator

Looks great, thanks! I just had a couple of questions inline. I'm new here, but do we have CI that tests this? I imagine this may be difficult or not possible?

We should definitely get CI that tests this if we don't have it!

Thanks @jameshcorbett. @gonsie, do you know whether our CI tests this? If not, we can at least write up an issue to get it covered at some point.

@mcfadden8 mcfadden8 merged commit 4b82583 into LLNL:develop Jan 31, 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.

2 participants