Resource Manager - #7
Conversation
Summary of ChangesHello @andre-merzky, I'm Gemini Code Assist1! 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 integrates a comprehensive resource management system into Rhapsody, migrating existing functionality and establishing a modular architecture. It provides a unified and extensible way to query and manage computational resources across diverse HPC environments, enabling Rhapsody to adapt to different batch systems and allocate resources effectively. The changes lay the groundwork for more robust and flexible resource scheduling within the framework by standardizing resource information and providing specific implementations for common HPC schedulers. Highlights
Using Gemini Code AssistThe 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
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a resource manager (RM) framework into Rhapsody, which is a significant and valuable addition. The implementation includes a base ResourceManager and several specific implementations for different batch systems like SLURM, LSF, and PBSPro.
My review has identified a few critical issues that need to be addressed. Firstly, there's a design flaw where several RM implementations (Cobalt, LSF, PBSPro, Torque) will fail at runtime because they don't implement the required _initialize method. Secondly, there are critical inconsistencies in how node data is structured and passed between methods, which will lead to runtime errors. I've provided detailed suggestions on how to refactor this for consistency.
I've also noted a potential command injection vulnerability and offered suggestions to improve code quality and maintainability, such as using standard Python features like Enum and @dataclass (addressing a question in your PR description), improving exception handling, and fixing some minor bugs related to logging and type hints.
|
I would feel strongly against introducing a ru dependency or using ru at all. I think over the years it proven to be a wrong design choice for the multiple reasons we discussed many times. |
Yes, Matteo, I know. The PS: this is a draft PR for a reason... Second reason: this is now rebased to Aymen's recent improvements. |
…cores_per_node
- Rename init_from_scratch() to _initialize() in PBSPro, Torque, Cobalt,
LSF so the base class actually calls the implementations
- Replace self._log.debug_1/debug_2 (PBSPro) and self._log.debug (LSF)
with module-level logger.debug
- Fix _get_cores_per_node: generator has no len(), use set→list instead
- Remove stray print("initialize") from _init_info
- Add logger.warning with exc_info in _parse_nodefile bare except
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add compactify_hostlist() and expand_hostlist() static methods to ResourceManager for converting between hostname lists and bracket notation (e.g., ['host001', 'host002'] <-> ['host00[1,2]']) - Add _build_brackets() and _minimal_prefix() helpers for compactify - Refactor get_hostlist() to use new _split_hoststring() + expand_hostlist() - Refactor get_hostlist_by_range() to format input and delegate to expand_hostlist(), removing duplicated expansion logic - Clean up slurm.py: remove unused imports, fix return type, fix _get_node_list() call, remove duplicate helper functions - Add comprehensive tests for all hostlist methods (37 total RM tests) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add get_partition_env() and release_partition_env() methods to base class - Add nodefile helper methods: _get_nodefile_path, _write_nodefile, _remove_nodefile - Implement partition env for Slurm (env vars: SLURM_NODELIST, SLURM_NNODES, etc.) - Implement partition env for PBSPro/Torque (PBS_NODEFILE, PBS_NUM_NODES) - Implement partition env for Cobalt (COBALT_NODEFILE, COBALT_PARTSIZE) - Implement partition env for LSF (LSB_DJOB_HOSTFILE) - Implement no-op partition env for Fork - Add comprehensive unit tests for all partition env methods Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Execution backends: - RadicalPilot: extract partition from resources, set pd.nodes and pd.prepare_env - Concurrent, Dask, Dragon V1/V2/V3: raise ValueError if partition specified Resource manager fixes: - Node.__post_init__: add early return when no rm_info provided - _filter_nodes: remove broken per-node core/gpu marking code - get_instance: separate "unknown RM" from "creation failed" errors Tests: - Add Node dataclass validation tests - Add _parse_nodefile and _get_cores_per_node tests - Add get_partition and get_instance error tests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove obvious comments that duplicate what the code shows - Fix _parse_nodefile docstring to match actual behavior (returns list of node names, not tuples) - Clarify _get_cores_per_node expects tuples from RM-specific parsing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Implement _parse_nodefile_and_cpn in base.py and use it in RMs - Refactor duplicate partition environment logic into _get_partition_env_with_nodefile - Improve factory method error handling to report all failures - Modernize type hints and add docstrings - Add comprehensive RM initialization tests (test_rm_initialization.py) - Fix bugs: check_nodes shadowing, tuple handling in get_node_list - Update Cobalt to auto-detect cores from localhost - Update PBSPro to auto-derive cores from nodefile if config missing
Resolved merge conflicts in: - src/rhapsody/__init__.py - src/rhapsody/backends/constants.py - src/rhapsody/backends/execution/concurrent.py - src/rhapsody/backends/execution/dask_parallel.py - src/rhapsody/backends/execution/dragon.py - src/rhapsody/backends/execution/radical_pilot.py All tests passing (207 passed, 23 skipped)
# Conflicts: # src/rhapsody/backends/execution/dragon.py
The edge execution backend was renamed to "orbit" (backed by radical.orbit instead of radical.edge) on the orbit branch, which lands first. Remove the stale edge.py + its test and unregister EdgeExecutionBackend so feature/rm does not re-introduce a duplicate backend alongside orbit. Update the noop docstring reference accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Makefile: fix target name uest-unit-dragon -> test-unit-dragon (the broken name meant `make test-unit-dragon` / the test-unit aggregate could not resolve it). - radical_pilot: copy resources before popping "partition" so the caller's dict is not mutated in place. - radical_pilot: shlex.quote partition env keys/values when building the `export` pre_exec directives, preventing shell injection / breakage on spaces and special characters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several changes across the codebase, including changing task submission logging to debug, adding a new NoopExecutionBackend for benchmarking, and implementing partition checks and configurations across various execution backends (Concurrent, Dask, Dragon, and Radical Pilot). Additionally, performance test thresholds were relaxed, and a new unit test environment was added to Tox. Feedback on the changes highlights potential issues with implicit boolean evaluation in task routing when values are falsy but valid, and a potential AttributeError in the Radical Pilot backend if the partition configuration is explicitly set to None.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Wrap the ConcurrentExecutionBackend ctor signature to the 100-col limit and drop a stray blank line in constants.py. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new NoopExecutionBackend for performance benchmarking, adds resource and partition configuration support to the RadicalPilot backend, and updates other execution backends to handle or reject resource parameters. It also refines task class selection, adjusts logging levels, increases a performance test timeout, and adds a unit test environment to tox.ini. Feedback focuses on avoiding permanent mutation of instance state in the RadicalPilot backend, and properly tracking and cancelling asyncio tasks in the new NoopExecutionBackend to ensure robust task lifecycle management.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ycle - radical_pilot: strip "partition" from a local copy of resources instead of reassigning self.resources, so self.resources stays intact for a retried _initialize (and the caller's dict is never mutated). - noop: track scheduled completion futures; cancel_task / cancel_all_tasks now cancel them and fire CANCELED (guarded against double terminal callbacks), and shutdown awaits cancel_all_tasks. submit_tasks now returns None to match the BaseBackend signature. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new NoopExecutionBackend for performance benchmarking, implements partition configuration support for the Radical Pilot backend, adds resource checks to other execution backends, and adjusts task routing checks to prevent misrouting on None values. The review feedback highlights critical memory leaks in the new NoopExecutionBackend where completed or cancelled tasks are never removed from the tracking dictionary, a potential crash in the Radical Pilot backend if the partition configuration is explicitly set to None, and a recommendation to use explicit 'is not None' checks instead of truthiness checks during task class selection to avoid misrouting falsy values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
- noop: generate a uid when missing and drop terminal tasks from the
registry (bounded memory for benchmarking); kept byte-identical to the
feature/orbit copy.
- task: route by "is not None" instead of truthiness so a present-but-
falsy field is not treated as absent.
- radical_pilot: treat an explicit "partition": None like an absent key
(``pop(..., None) or {}``) so partition.get() cannot hit AttributeError.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new NoopExecutionBackend for performance benchmarking, adds partition configuration support to the RadicalExecutionBackend, and updates several other backends to handle resource/partition validation. Additionally, task type determination in task.py is improved to check for explicit non-None values, logging is tuned, and a unit test environment is added to tox.ini. The review feedback suggests improving the new NoopExecutionBackend by aligning its task_state_cb signature with other backends to prevent type errors and adding a create factory classmethod for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
ConcurrentExecutionBackend.__init__ gained a `resources: dict | None` parameter (partition support), and PEP 604 unions are evaluated eagerly on Python 3.9 at definition time -> TypeError on import, breaking the 3.9 CI job. Defer annotation evaluation, matching the other backend modules. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
task_state_cb(self, task, state) to match the other backends, and add the create() classmethod factory for parity. Kept byte-identical to the feature/orbit copy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Superseded by #76 (feature/partitions-orbit): the consumer-side partition hooks were ported there on top of the orbit base, per the rhapsody_rm CONTRACT.md split (rhapsody_rm produces the partition spec, rhapsody consumes it). The RM implementation itself lives in the rhapsody_rm repo now. |
3 participants
This PR moves the RP resource manager into Rhapsody. The API is trivial: once created via
get_instance(cfg), the returned RM will have anrm.infoattached with information such as cores per node etc, andrm.node_listwith the node list (duh!). The node list can be partitioned like this:There are a couple of FIXMEs I'd like to get feedback on (in scope, out of scope, etc). Also, this uses RU's typed dict for defining some data structures. But we don't use RU anymore it seems, so what's the approach: not defining data structures (what about default values and verification)? Use
@dataclass? Reimplement what we had?NOTE: this is a draft PR for discussion, for now please ignore the gemini comments.