Improve testing robustness on SLURM machines #381
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MattToast
requested changes
Oct 3, 2023
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #381 +/- ##
===========================================
- Coverage 90.38% 90.29% -0.10%
===========================================
Files 60 60
Lines 3839 3864 +25
===========================================
+ Hits 3470 3489 +19
- Misses 369 375 +6
|
…_create_standard_twice
…cked GPU" This reverts commit 762db80.
Much of the need to check the type and value of the the nodes property in QsubBatchSettings is because there are two technically valid, but not quite equivalent ways of setting the number of nodes. Now, we check at various points that the both 'select' and 'nodes' is not set. Additionally, both routes can be used to set the internal _nodes property if it needs to be accessed within Python
MattToast
requested changes
Dec 7, 2023
MattToast
reviewed
Dec 7, 2023
Further refactors the way that QsubBatchSettings is used and accessed to streamline the logic and make it fail faster if users attempt to set the number of nodes in multiple different ways
al-rigazzi
reviewed
Dec 8, 2023
| self._ncpus = ncpus | ||
| self._resources = resources or {} | ||
|
|
||
| resource_nodes = self.resources.get("nodes", None) |
There was a problem hiding this comment.
I think this might need to be resource_nodes = self._resources.get("nodes", None)
There was a problem hiding this comment.
I think the assignment on 74 is actually wrong (or rather it doesn't go through the new setter method)
| if num_nodes: | ||
| self._nodes = int(num_nodes) | ||
| self.set_resource("nodes", num_nodes) |
There was a problem hiding this comment.
I think casting num_nodes to str would make the resource dict easier to handle, as it would be of type dict[str, t.Optional[str]] instead of having to add int as another value type.
There was a problem hiding this comment.
Hrmm, I would agree if num_nodes was the only int option, but I think there's other fields like ncpus, ngpu, etc. that should be int, which can get converted to a string
MattToast
requested changes
Dec 8, 2023
We now check a number of extra cases that the user can follow when trying to specify resources. These include: - Validating prior to assignment, the additio of a resource to the dictionary - Validating the types of keys and their values to be str or int
MattToast
requested changes
Dec 9, 2023
MattToast
added a commit
that referenced
this pull request
Dec 15, 2023
Bumps the required number of nodes in the test docs from 3 to 4 as required by the tests in #381 and #426. [ committed by @MattToast ] [ reviewed by @ashao ]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some aspects of the testing were failing on SLURM machines due to (1) inconsistent assumptions about how the tasks should be run and (2 suspected hidden race conditions potentially related to the filesystem that caused non-idempotent behavior when running the tests. These defects were ameliorated by ensuring that the failing tests used only a single task and creating a separate run directory for every variant of the test.