-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/add requested vram #23
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
Conversation
f585e97 to
2b861be
Compare
…ts and refactor the file into a class. Add anonymization to DatabaseConnection
… the warning is raised in the output
…placed by sort_and_filter
| *.ipynb filter=strip-notebook-output | ||
| # keep the output of the following notebooks when committing | ||
| SlurmGPU.ipynb !filter=strip-notebook-output No newline at end of file | ||
| *.ipynb filter=strip-notebook-output No newline at end of 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.
why are we not keeping the SlurmGPU output now?
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.
The list of notebooks to not filter out is handled by the script clean_notebook.sh so this was unnecessary.
| from .errors import JobPreprocessingError | ||
|
|
||
|
|
||
| class Preprocess: |
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 don't see the point of making this a class, as it just adds a layer of complication (and again, the other code for queries etc will have to be updated for no reason). The functions work fine, and we don't need to store a state. We could just pass in anonymize into the function if we need to. Also, maybe anonymize is not the best word since it works for replacing user names but it isn't exactly clear that it also removes logs. Maybe add a separate parameter for removing logs, or rename this?
|
We probably shouldn't merge breaking changes (like the new parameters in EF and the preprocess class) because every other piece (even if not merged) would need to be changed, and we don't have time to do that. Even reports, which should be one of the main components. |
…ations to keep the output for an analysis notebook
MisterArdavan
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.
I applied the changes I had made in preprocess to match the new functions in utilities. I also added the two analysis notebooks with their outputs anonymized to the list of notebooks that we keep their outputs. The only thing that remains is to add a short project description to the main project README that also references the notebooks.
| *.ipynb filter=strip-notebook-output | ||
| # keep the output of the following notebooks when committing | ||
| SlurmGPU.ipynb !filter=strip-notebook-output No newline at end of file | ||
| *.ipynb filter=strip-notebook-output No newline at end of 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.
The list of notebooks to not filter out is handled by the script clean_notebook.sh so this was unnecessary.
…s. Keep the output of this notebook.
…sh their outputs to the repo
…eature/add-requested-vram
Adds
requested_vramto the metrics calculated for users and PI groups and replaces efficiency and score metrics based onvram_constraintalone.This branch is created off
feature/high-cpu-mem-analysis(#22).