Skip to content

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

Merged
merged 16 commits into from
Apr 23, 2025
Merged

Development #14

merged 16 commits into from
Apr 23, 2025

Conversation

gjbex
Copy link
Owner

@gjbex gjbex commented Apr 23, 2025

Summary by Sourcery

Refactor and improve the Hydra configuration and process monitoring scripts, adding more robust error handling and configuration flexibility

New Features:

  • Added a debug script for Hydra configuration
  • Introduced a new configuration file for file output in Hydra script

Bug Fixes:

  • Fixed various minor issues in process monitoring and shell interaction scripts
  • Corrected argument parsing and error handling in monitoring script

Enhancements:

  • Simplified and improved error handling in process monitoring script
  • Refactored Hydra configuration files to use more concise and flexible configuration

Documentation:

  • Updated README.md with more detailed usage instructions for Hydra script

Chores:

  • Updated environment dependencies to include hydra-core
  • Cleaned up and simplified configuration file structures

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

sourcery-ai bot commented Apr 23, 2025

Reviewer's Guide by Sourcery

This pull request includes several enhancements and refactorings. The monitor.py script has been improved with better error handling and more concise code. The shell_interaction.ipynb notebook has been updated to reflect changes in the sh library and provide clearer examples. The gen_rand.py script has been integrated with Hydra for configuration management, offering more flexibility and control over the random number generation process.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Refactored the monitor.py script to improve readability and error handling.
  • Simplified the get_read_open_files and get_write_open_files functions using list comprehensions.
  • Added error handling for the case where the process ID does not exist.
  • Handled ZombieProcess exceptions when iterating through child processes.
  • Replaced file.mode != 'r' with a more robust check for write access in get_write_open_files.
  • Used a context manager to ensure the output file is properly closed.
  • Added a return code to the main function.
  • Removed unused mem_perpent metric.
source-code/processes/monitor.py
Updated the shell_interaction.ipynb notebook to reflect changes in the sh library and improve examples.
  • Removed decoding of stdout.
  • Modified examples to use result.split('\n') instead of cmd.stdout.decode(encoding='utf8').split('\n').
  • Updated examples to use print(sh.cat(...)) instead of sh.cat(...).
  • Added _in argument to sh.grep.
  • Added _iter argument to sh.sort.
  • Corrected the subprocess.run example to use '.ipynb' instead of '.py'.
hands-on/shell_interaction.ipynb
Integrated Hydra into the gen_rand.py script for configuration management.
  • Added Hydra decorators and configuration files.
  • Replaced argument parsing with Hydra's configuration system.
  • Improved error handling and logging.
  • Added the ability to specify an output file via configuration.
  • Removed the verbose option.
  • Added debug.py to print the configuration settings.
source-code/hydra/gen_rand.py
source-code/hydra/conf/distr/gauss.yaml
source-code/hydra/conf/distr/uniform.yaml
source-code/hydra/conf/config.yaml
source-code/hydra/debug.py
source-code/hydra/conf/file_config.yaml
source-code/hydra/README.md
Updated the environment file.
  • Added hydra-core to the environment file.
environment.yml

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@gjbex gjbex merged commit c965d58 into master Apr 23, 2025
Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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']
Copy link

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
Copy link

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.
Copy link

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:
Copy link

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"

Suggested change
Run with configuratino file settings:
Run with configuration file settings:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant