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

Update SLURM builder #323

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Update SLURM builder #323

wants to merge 30 commits into from

Conversation

jarlsondre
Copy link
Collaborator

@jarlsondre jarlsondre commented Feb 10, 2025

Summary

Updates to the itwinai SLURM builder. See issue #288 for more information.

Brief overview:

  • Add SLURM Builder to CLI
  • Use tempdir when --no-save-script is active
  • Add some documentation for the builder in the existing SLURM command documentation
  • Add some tests for the SLURM builder
  • Use pytest's tmp_path instead of tempfile's TemporaryDirectory wherever possible
  • Remove old slurm.sh and runall.sh files from use cases
  • Add training command with overridable string, removing the need for separate slurm.py files in EURAC and Virgo
  • General clean up such as changing the type hints in the TorchTrainer etc.
  • Possibly some more small stuff 😁

Related issues : #288, #316 #318, #319, #320, #324, #325

@jarlsondre jarlsondre requested review from matbun and removed request for matbun February 13, 2025 08:26
@jarlsondre jarlsondre self-assigned this Feb 13, 2025
@jarlsondre jarlsondre added documentation Improvements or additions to documentation enhancement New feature or request clean up labels Feb 13, 2025
@jarlsondre jarlsondre modified the milestones: itwinai 0.2, itwinai 0.3 Feb 13, 2025
@jarlsondre jarlsondre marked this pull request as ready for review February 13, 2025 08:33
@jarlsondre jarlsondre changed the title [DRAFT] Update SLURM builder Update SLURM builder Feb 13, 2025
Comment on lines +40 to +42
typer.Option(
help=("Which directory to search for the scalability metrics in.")
),
Copy link
Collaborator

Choose a reason for hiding this comment

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

our formatters will keep fighting on this forever hahaha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Haha yeah I think I have my line length a bit shorter than yours. Mine doesn't use the project-wide one I think

@app.command(
context_settings={"allow_extra_args": True, "ignore_unknown_options": True}
)
def generate_slurm():
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be nice to replicate arguments and options of the argument parser here as well. This would generate a nice help page, helping the users to understand what options they can use and what type they receive. You can take inspiration from exec_pipeline

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is possible and might be better since the Typer help page is pretty, but since I am using the argparser, doing -h actually works atm and will tell you all the arguments that exist. The format will be slightly different from Typer's, though.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see two limitations with this solution. First, I guess that if you use --help instead of -h, this help page will not appear -- a bit inconsistent and may generate confusion. Second, the CLI reference is not generated in the dedicated docs page

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Regarding the first limitation: This is possible to change by simply setting add_help_option=False.

The CLI reference might not be as easy to fix. We could just add them as arguments, but there are 23 of them in total. Looking at the other Annotated variables they take between 1 and 8 lines each, where most seem to take 5-6. This would result in around 100-150 lines of variables. Certainly doable and might be the best fix, but certainly worth thinking about. What do you think? Should I add them? 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose the biggest downside would be that we would have all the descriptions in two places, as well as all the variables. We could solve this by removing the descriptions from the argparser and maybe having an automatic test (in tests/) that checks that the params are the same between the argparser and the function? Just thinking out loud here.

Another option is to change the parsing mechanism to use the newest parser. That way we wouldn't have the whole double thing. The disadvantage is that it could potentially be a lot of work for something that already works the way it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean up documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants