-
Notifications
You must be signed in to change notification settings - Fork 213
Represent segments/checkpoints in Python, use in status CLI #5078
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
Represent segments/checkpoints in Python, use in status CLI #5078
Conversation
6daeaa1 to
ba9942c
Compare
knelli2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you include the changes to Main that actually change the checkpoint structure in this PR?
| # Keep only latest in a series of segments | ||
| job_data[["SegmentsDir", "SegmentId"]] = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a column that prints what segment we are on? I think that could be useful. Then we don't have to include it in the job name like SpEC does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that before but didn't find it particularly useful. I can add it if you want though. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For long runs I think it'd be useful. It's a very quick way to know how many times the simulation has restarted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right but is it useful to know the number of restarts? I don't know. Remember we have limited space in the terminal, so adding a column means things like the job name get truncated further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it should be an option. Everybody is going to want to see different things, so maybe (for a later PR) we should take the fields to print as arguments so users can customize them in their spectre.yaml file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea that would be nice
|
Moved the changes to Main to #5079 |
8cecf17 to
c1257db
Compare
|
@wthrowe could you take this? |
| self.test_dir = Path( | ||
| unit_test_build_path(), "support/DirectoryStructure" | ||
| ) | ||
| self.test_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete any preexisting files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok fixed
c1257db to
63f6b9a
Compare
|
Agreed. I don't see @knelli2's request as blocking. |
Proposed changes
Preparation for #4996 to make that PR a bit smaller.
Upgrade instructions
Code review checklist
make docto generate the documentation locally intoBUILD_DIR/docs/html.Then open
index.html.code review guide.
bugfixornew featureif appropriate.Further comments