feat: Correct behavior of matrix fallback priority, error on duplicate matrix selectors. #110
Conversation
bdice
changed the title
|
|
||
| if not found: | ||
| break | ||
| else: |
There was a problem hiding this comment.
On the off chance that this is new to you: Python has a for/else construct that can be used to simplify things where you would otherwise need a loop with a "found" variable. It doesn't come up often in Python usage, but it's nice when it fits the problem cleanly. I was able to eliminate the found variable here with that approach.
There was a problem hiding this comment.
Thanks for pointing this out, this was new to me.
There was a problem hiding this comment.
This looks great to me, thanks for the test cases.
I pulled this and installed it, and tried the test case from cudf that led to this:
rapids-dependency-file-generator \
--output conda \
--file-key test_python \
--matrix "cuda=12.2;arch=aarch64;py=3.10;dependencies=oldest"Saw that produce output with cupy==12.2.0, meaning it matched the first matching entry in https://github.com/rapidsai/cudf/blob/9b4c4c721c399bae9e88733da79daa1a10644481/dependencies.yaml#L715-L724, exactly as we'd expect.
|
Thanks for the review @jameslamb! I'm going to merge this and deploy it so that we can get working nightlies for cudf branch-24.10 before code freeze. |
# [1.15.0](v1.14.0...v1.15.0) (2024-09-23) ### Features * Correct behavior of matrix fallback priority, error on duplicate matrix selectors. ([#110](#110)) ([298c61e](298c61e))
|
🎉 This PR is included in version 1.15.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The README says,
However, it seems that the implemented behavior does not align with this. Only the fallback matrix worked in this way. This PR fixes the implementation to match the README (and our expectations).
We also decided it would be nice to fail if there are duplicate matrix selectors, since only the first one would be usable.
I added tests covered by both of these features/fixes.