-
Notifications
You must be signed in to change notification settings - Fork 13
Development #14
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
Development #14
Conversation
The hydra configuration is stored in the output directory anyway, so having this flag will detract from that hydra feature.
…gramming into development
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Reviewer's Guide by SourceryThis pull request includes several enhancements and refactorings. The No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @gjbex - I've reviewed your changes - here's some feedback:
Overall Comments:
- The
monitor.py
script could benefit from more robust error handling, especially around process interactions. - The
shell_interaction.ipynb
notebook could benefit from a clearer narrative flow to better explain the concepts. - The hydra example could benefit from a more detailed explanation of the configuration options in the README.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pass | ||
except: | ||
pass | ||
open_files = [file.path for file in process.open_files() if file.mode != 'r'] |
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.
question (bug_risk): File size information is no longer captured in write-open-files.
Earlier the file size was appended using file.stat(). Verify that omitting this detail is intentional.
inactive = [] | ||
if not options.affinity: | ||
inactive.append('affinity') | ||
if not options.files: | ||
inactive.extend(('read_files', 'write_files')) | ||
metrics = define_actions(inactive) | ||
file = open(options.output_file, 'w') if options.output_file else sys.stdout | ||
found_process = False |
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.
suggestion: Redundant 'found_process' flag.
The variable 'found_process' is initialized but never updated, making its later check ineffective. It would be clearer to remove it or implement the intended logic.
Suggested implementation:
Review the rest of the function to ensure there are no further references to "found_process". If there are, you'll need to remove or adjust those accordingly.
@@ -14,23 +15,50 @@ multiruns and so on. | |||
1. `config.yaml`: configuration file with the defaults. | |||
1. `distr/gauss.yaml`: configuration file for the Gaussian distirubtion. |
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.
issue (typo): Typo: "distirubtion" should be "distribution"
Suggested implementation:
1. `distr/gauss.yaml`: configuration file for the Gaussian distribution.
1. `distr/uniform.yaml`: configuration file for the uniform distribution.
|
||
## How to use it? | ||
|
||
Run with configuratino file settings: |
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.
issue (typo): Typo: "configuratino" should be "configuration"
Run with configuratino file settings: | |
Run with configuration file settings: |
Summary by Sourcery
Refactor and improve the Hydra configuration and process monitoring scripts, adding more robust error handling and configuration flexibility
New Features:
Bug Fixes:
Enhancements:
Documentation:
Chores: