fix: max resources are what the user requested #119
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the resource allocation logic to use the user-requested resources as the upper bound for binary search, rather than a calculated value based on QoS policies. The key change is renaming the cores parameter to requested_cores for clarity and adjusting when QoS policy selection occurs.
Key Changes:
- Parameter renamed from
corestorequested_coresthroughout the codebase for improved clarity - QoS policy selection moved from before to after the binary search completes
- Upper bound for binary search now uses
requested_resourcesdirectly instead ofmin(2x max workflow, QoS max cores)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_planner.py |
Updates test calls to use the renamed parameter requested_cores instead of cores |
src/socm/planner/heft_planner.py |
Refactors _plan_with_qos_optimization to accept requested_resources, use it as the binary search upper bound, and perform QoS validation after finding the optimal core count; also improves logging granularity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| else: | ||
| raise ValueError( | ||
| f"Cannot meet {self._objective} min deadline with {requested_resources} cores. " |
There was a problem hiding this comment.
The error message format is inconsistent. Line 77 uses "deadline of {self._objective} minutes" while line 150 uses "{self._objective} min deadline". These should use the same format for consistency.
| f"Cannot meet {self._objective} min deadline with {requested_resources} cores. " | |
| f"Cannot meet deadline of {self._objective} minutes with {requested_resources} cores. " |
| else: | ||
| raise ValueError( | ||
| f"Cannot meet {self._objective} min deadline with {requested_resources} cores. " | ||
| f"Please increase deadline or increase requested cores." |
There was a problem hiding this comment.
The error message suggests to "increase requested cores", but the actual problem is that the requested cores are insufficient to meet the deadline. The suggestion should be "increase requested resources" or just remove this suggestion since increasing the deadline is already mentioned and is the more appropriate solution when the deadline cannot be met with the given resources.
| f"Please increase deadline or increase requested cores." | |
| f"Please increase deadline or increase requested resources." |
| assert suitable == QosPolicy(name="long", max_walltime=240, max_jobs=10, max_cores=400) | ||
| with pytest.raises(ValueError): | ||
| suitable = planner._find_suitable_qos_policies(cores=410) | ||
| suitable = planner._find_suitable_qos_policies(requested_cores=410) |
There was a problem hiding this comment.
Variable suitable is not used.
| suitable = planner._find_suitable_qos_policies(requested_cores=410) | |
| planner._find_suitable_qos_policies(requested_cores=410) |
No description provided.