378 test queue parameter #380
Conversation
|
Thank you @RamonAra209! |
tests/test_queue.py
Outdated
| valid_queues = [] | ||
| out = "".join(os.popen("bqueues -u $(whoami) -o 'QUEUE_NAME NJOBS PEND RUN SUSP STATUS'").read()).split("\n") | ||
| out = [l for l in out if len(l) != 0] | ||
| out = [l.split(" ") for l in out] |
There was a problem hiding this comment.
out seems to be both List[str] and List[List[str]] here. It might be best if we didn't overload it like that.
tests/test_queue.py
Outdated
|
|
||
| def get_lsf_queues() -> List[str]: | ||
| valid_queues = [] | ||
| out = "".join(os.popen("bqueues -u $(whoami) -o 'QUEUE_NAME NJOBS PEND RUN SUSP STATUS'").read()).split("\n") |
There was a problem hiding this comment.
bqueues appears both here and in SCHEDULER_COMMANDS. Is the latter not used?
There was a problem hiding this comment.
Done, now using the dictionary as intended
| slurm_queues = get_slurm_queues() | ||
| lsf_queues = get_lsf_queues() | ||
|
|
||
| queues.extend(slurm_queues) | ||
| queues.extend(lsf_queues) |
There was a problem hiding this comment.
This is kind of a general note, but we know what scheduler we have from execparams.
That's not the point I wanted to make though. The idea of running all possible get_*_qeues() and merging the results with the assumption that at most one of them will return non-empty results probably works. But it does so in an unnecessarily twisted way and it does rely on an assumption that isn't necessary to make or reason through.
| if len(slurm_queues) != 0: | ||
| scheduler = "slurm" | ||
| elif len(lsf_queues) != 0: | ||
| scheduler = "lsf" |
There was a problem hiding this comment.
I see.
So execparams is there to parametrize executors when multiple executors are available on a system. For example, on a SLURM system, a test with an execparams parameter will be invoked multiple times for all combinations of executor in ["local", "batch-test", "slurm"] \crossproduct launcher in ["single", "multiple", "mpirun", "srun"}.
If you ignore execparams and detect what's installed the way it's done here, it will work, but it will run the same test multiple times for no good reason.
Instead, we should run this test on only one of the launchers (the launcher doesn't matter because we don't actually care about launching a job in this test) and using all executors. So something like if execparams.launcher == 'single' then do what we need to do with the assumption that our scheduler is execparams.executor.
There was a problem hiding this comment.
Can you take a look at my implementation? I kept the way I was detecting it, but am now only running the test when execparams.launcher == "single"
There was a problem hiding this comment.
It's not about detecting the LRM on the system but the fact that we test multiple executors on that system. So even if you restrict it to the single launcher, it will still be repeated for the local, batch-test, and whatever PSI/J detected to be the scheduler.
You could remove execparams, but then you risk not having access to other necessary parameters that might be set by the users that set up the tests. By the way, you may want to use execparams.custom_attributes, since some systems require setting various things, like an account or project.
tests/test_queue.py
Outdated
| scheduler = "lsf" | ||
|
|
||
| if len(queues) < 2: | ||
| pytest.raises(Exception("Need at least two queues to perform this test")) |
There was a problem hiding this comment.
I think you are looking for pytest.skip instead.
pytest.raises is used to test that a block of code throws a specific exception. For example,
def test_that_division_by_zero_correctly_raises_exception():
with pytest.raises(ZeroDivisionError):
1 / 0In other words you use it to check that you test throws an exception. If you had 1 / 1 instead of 1 / 0, pytest.raises would actually cause the test to fail because it did not throw the exception that was expected.
tests/test_queue.py
Outdated
|
|
||
| job1 = make_job(test_queues[0]) | ||
| executor.submit(job1) | ||
| qstat = get_queue_info(scheduler) |
There was a problem hiding this comment.
Most of these commands accept a job id as an argument to only return info about a specific job.
There was a problem hiding this comment.
Done, updated the qstat equivalents to query just for the job.
tests/test_queue.py
Outdated
| job1_qstat_entry = [l for l in qstat if job1._native_id in l][0] | ||
| assert test_queues[0] in job1_qstat_entry |
There was a problem hiding this comment.
We might want to go a bit further than just checking if the queue name is somewhere in the qstat output. It's quite possible that we might have a queue named "test" and the word "test" appearing in an unrelated place in the qstat output.
There was a problem hiding this comment.
Done, I applied some custom formatting on the qstat equivalents, so they now only report the queue name. This allows me to keep the existing assert logic
Ex:
$ bjobs -o "queue" 4775749
QUEUE
pbatch
Codecov Report
@@ Coverage Diff @@
## main #380 +/- ##
==========================================
- Coverage 69.26% 68.70% -0.56%
==========================================
Files 74 75 +1
Lines 3208 3285 +77
==========================================
+ Hits 2222 2257 +35
- Misses 986 1028 +42
|
No description provided.