Allow MPI variables to be optional

[vishalbh]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the configurability of the system by making key MPI-related variables optional. This change addresses scenarios where MPI might not be in use, preventing unnecessary variable definitions and potential configuration errors. The system now dynamically determines the need for MPI variable definition based on executable usage, leading to a more robust and adaptable framework. Supporting test cases have been adjusted to validate this new behavior.

Highlights

• MPI Variable Optionality: The core MPI-related variables, specifically "n_ranks", "n_nodes", and "processes_per_node", have been transitioned from required to optional. This change allows for more flexible configuration, especially in environments where MPI is not always utilized.
• Conditional MPI Variable Definition: The system now intelligently determines whether MPI variables need to be defined. It checks if any executable within a workload is configured to use MPI, and only then proceeds to define the associated MPI variables.
• Test Suite Updates: Numerous test cases across workspace_concretize, workspace_manage, concretize_builtin, and end-to-end tests have been updated to reflect the new optional status of MPI variables. This includes explicitly setting these variables to empty strings in some test commands and providing default calculation logic in others.
• Test Refactoring and Fixes: A minor typo was corrected in lib/ramble/ramble/test/application.py (changing templates to template). Additionally, several experiment_set tests were refactored by updating workload context names to improve clarity and prevent potential naming conflicts.

Changelog

• lib/ramble/ramble/keywords.py
  • Changed the type of n_ranks, n_nodes, and processes_per_node from required to optional.
• lib/ramble/ramble/test/application.py
  • Corrected a key from templates to template in a test executable definition.
• lib/ramble/ramble/test/cmd/workspace_concretize.py
  • Modified generate-config commands to explicitly set n_ranks and n_nodes as empty strings.
• lib/ramble/ramble/test/cmd/workspace_manage.py
  • Added explicit empty string assignments for n_nodes and n_ranks in a generate-config command and removed an assertion related to processes_per_node.
• lib/ramble/ramble/test/concretize_builtin.py
  • Added explicit empty string assignments for n_ranks and n_nodes in a generate-config command.
• lib/ramble/ramble/test/end_to_end/analyze_upload.py
  • Added a default calculation for n_ranks in a test configuration.
• lib/ramble/ramble/test/end_to_end/formatted_executables.py
  • Added a default calculation for n_ranks in a test configuration.
• lib/ramble/ramble/test/experiment_set.py
  • Updated workload context names and corresponding assertions in several test functions.
• var/ramble/repos/builtin/base_classes/application-base/base_class.py
  • Implemented conditional definition of MPI variables based on whether MPI is used by any executable in the workload.

Activity

• No activity has been recorded for this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request makes MPI-related variables like n_ranks, n_nodes, and processes_per_node optional. The logic is updated to only define these variables if an MPI command is actually used in an experiment. This is a great change for flexibility, especially for non-MPI workloads. The tests have been updated accordingly to reflect this change. I've found one critical issue in the implementation that needs to be addressed.

[douglasjacobsen]

/gcbrun

[vishalbh]

/gcbrun

[douglasjacobsen]

/gcbrun

[douglasjacobsen]

Can we make a helper method in the class to return if MPI is required for the workload, and then call it here?

[douglasjacobsen]

Instead of editing the tests, can we use the helper method that I mentioned below in the base class, and then conditionally defined these in workspace.add_experiments rather than setting them explicitly here?

The main reason is because we want the user experience from generate-config or workspace manage experiments to be that users get a mostly functional workspace when they run these. And we should be able to identify if we need to define the MPI vars or not when they are creating experiments.

[douglasjacobsen]

Another option actually is to write two helper functions. One to determine if MPI is required, and another to update the keywords to make the MPI variables required (if MPI is required).

Then this code:
https://github.com/GoogleCloudPlatform/ramble/blob/develop/lib/ramble/ramble/workspace/workspace.py#L1352

should automatically handle defining them.

[douglasjacobsen]

/gcbrun

[douglasjacobsen]

/Gemini review

[gemini-code-assist]

Code Review

This pull request refactors how MPI-related variables are handled within the Ramble framework, making n_ranks, n_nodes, and processes_per_node optional by default and dynamically marking them as required when MPI is detected for a workload. It introduces new methods for checking and requiring MPI variables and improves error handling for undefined workloads by catching WorkloadNotDefinedError. The review identified several areas for improvement, including replacing silent try...except Exception: pass blocks with more specific error handling or logging, removing unused code (make_keys_required method), and cleaning up debugging print statements and commented-out code in test files.

[douglasjacobsen]

/gcbrun

[douglasjacobsen]

/gemini review

[gemini-code-assist]

Code Review

This pull request modifies the requirement status of MPI-related keywords, making n_ranks, n_nodes, and processes_per_node optional by default and introducing logic to enforce them only when a workload requires MPI. It also improves error handling by replacing terminal log calls with a new WorkloadNotDefinedError and updates the workspace logic to handle version validation more gracefully during experiment addition. Feedback was provided to improve the readability of the is_mpi_required method by combining nested conditional statements.

[douglasjacobsen]

/gcbrun