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

(Re)process runs from the GUI #285

Merged
merged 5 commits into from
Jul 31, 2024
Merged

(Re)process runs from the GUI #285

merged 5 commits into from
Jul 31, 2024

Conversation

takluyver
Copy link
Member

Another attempt at this, now that processing happens in Slurm.

image

Closes #39

@takluyver takluyver added the enhancement New feature or request label Jul 10, 2024
@takluyver takluyver force-pushed the feat/reprocess-gui-3 branch from d2fb95e to 9327aa5 Compare July 10, 2024 14:44
@tmichela
Copy link
Member

Could you also add an option on right click?

@tmichela
Copy link
Member

it'd also be nice to check if new variables exist in the context file that were never processed, otherwise it will always be skipped, unless one selects all variables.

@takluyver
Copy link
Member Author

I'll add a right-click option. 👍

For new variables: would it be a decent compromise to update the variables whenever you successfully save the context file in the GUI? Loading the context file often takes a few seconds, so doing it when showing this dialog either means an awkward pause before showing it, or a janky update after the list shows up, neither of which seem like great UX.

I'm also wondering about an option to process only variables which don't already have results, to fill in newly added variables.

@takluyver takluyver force-pushed the feat/reprocess-gui-3 branch from 9327aa5 to 9c0d5fe Compare July 19, 2024 17:28
@tmichela
Copy link
Member

For new variables: would it be a decent compromise to update the variables whenever you successfully save the context file in the GUI?

It is really that long to validate the file? 🤔
What do you think of having a "load new variables" button then on the widget? or a "process new variable" checkbox?

@tmichela
Copy link
Member

I'm thinking that I would be nice to have a visual indicator of the runs processing, particularly in this case of processing from the gui. But I think we don't have a way to monitor slurm jobs status yet, right?

Co-authored-by: Thomas Michelat <[email protected]>
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 26.41509% with 156 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@1a364da). Learn more about missing BASE report.

Files with missing lines Patch % Lines
damnit/gui/process.py 21.65% 123 Missing ⚠️
damnit/gui/main_window.py 25.00% 21 Missing ⚠️
damnit/gui/table.py 37.50% 5 Missing ⚠️
damnit/backend/extract_data.py 50.00% 3 Missing ⚠️
damnit/backend/extraction_control.py 50.00% 3 Missing ⚠️
damnit/ctxsupport/ctxrunner.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #285   +/-   ##
=========================================
  Coverage          ?   74.74%           
=========================================
  Files             ?       32           
  Lines             ?     4835           
  Branches          ?        0           
=========================================
  Hits              ?     3614           
  Misses            ?     1221           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@takluyver
Copy link
Member Author

It is really that long to validate the file? 🤔

It's now common/normal to use a different Python environment for evaluating the context file from running the DAMNIT application. So validating it means launching a subprocess and then loading all the modules it imports. Between GPFS, which isn't great for lots of small files, and xarray, which gets its __version__ in a convenient but inefficient way, it can easily be a couple of seconds. Not a long time, but definitely not instant for GUI code, hence #291.

I'm thinking that I would be nice to have a visual indicator of the runs processing

Yup, I'm also planning that, but I wanted to do this first.

@tmichela
Copy link
Member

I'm happy with the current state, feel free to merge.
We can address the other points later if they are needed.

LGTM

@takluyver
Copy link
Member Author

Thanks for the review!

@takluyver takluyver merged commit aa07dcc into master Jul 31, 2024
6 checks passed
@takluyver takluyver deleted the feat/reprocess-gui-3 branch July 31, 2024 13:12
@takluyver takluyver mentioned this pull request Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the users to reprocess runs from the GUI
2 participants