rapidsai / dependency-file-generator

https://pypi.org/project/rapids-dependency-file-generator/
Apache License 2.0
15 stars 13 forks source link

test: add tests on handling of duplicate matrix selectors #102

Closed jameslamb closed 4 months ago

jameslamb commented 4 months ago

Contributes to https://github.com/rapidsai/build-planning/issues/31.

Over in https://github.com/rapidsai/devcontainers/pull/365, I'm proposing a change that depends on how rapids-dependency-file-generator handles duplicates in --matrix selectors.

See https://github.com/rapidsai/devcontainers/pull/365#discussion_r1687121294.

This proposes adding tests covering that behavior, to reduce the risk of accidentally removing that support in future refactorings.

jameslamb commented 4 months ago

This is fine, but if it was me, I would have modified https://github.com/rapidsai/devcontainers/pull/365#discussion_r1687121294 to detect if the user is overriding cuda_suffixed to avoid passing more than one value.

The simplest implementation I can think of involves tracking the defaults as a bash associative array, splitting each --matrix-entry value by = into (key, value), and then indexing into that associative array with them.

That was more complexity than I was comfortable with for this purpose, but I'm willing to try it. Let's please not hold up https://github.com/rapidsai/devcontainers/pull/365 or this PR for that though. I'd like to try to get https://github.com/rapidsai/devcontainers/pull/365 and the corresponding dependencies.yaml changes out before code freeze for 24.08 (end of day tomorrow).

Let's explicitly document this behavior too.

Document what, where? I don't understand what you're asking for, sorry.

KyleFromNVIDIA commented 4 months ago

Document what, where? I don't understand what you're asking for, sorry.

Document the fact that the last duplicate is the one that DFG actually uses, which is what this PR is all about.

jameslamb commented 4 months ago

Document the fact that the last duplicate is the one that DFG actually uses, which is what this PR is all about.

Sure, but I'm not sure where you were looking for docs to be added (doc strings in code? the README? one of those, but over in the devcontainers code relying on this?).

I pushed https://github.com/rapidsai/dependency-file-generator/pull/102/commits/cfe81b417f572e1f7d3870c4ac64b35e034804b6 adding a line to the README, is that what you were looking for?

KyleFromNVIDIA commented 4 months ago

The README, because that's what the end user is going to see.

jameslamb commented 4 months ago

Gotchu! Ok yeah no problem, I agree that's a good addition.

Thanks for the quick review, always appreciated!

GPUtester commented 2 months ago

:tada: This PR is included in version 1.15.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: