Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

jameslamb
Copy link
Member

Contributes to rapidsai/build-planning#31.

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

See rapidsai/devcontainers#365 (comment).

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

Copy link
Contributor

@KyleFromNVIDIA KyleFromNVIDIA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but if it was me, I would have modified rapidsai/devcontainers#365 (comment) to detect if the user is overriding cuda_suffixed to avoid passing more than one value.

Let's explicitly document this behavior too.

@jameslamb
Copy link
Member Author

This is fine, but if it was me, I would have modified rapidsai/devcontainers#365 (comment) 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 rapidsai/devcontainers#365 or this PR for that though. I'd like to try to get rapidsai/devcontainers#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
Copy link
Contributor

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
Copy link
Member Author

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 cfe81b4 adding a line to the README, is that what you were looking for?

@KyleFromNVIDIA
Copy link
Contributor

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

@jameslamb
Copy link
Member Author

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

Thanks for the quick review, always appreciated!

@jameslamb jameslamb merged commit 535f0a9 into rapidsai:main Jul 23, 2024
4 checks passed
@jameslamb jameslamb deleted the test-duplicate-handling branch July 23, 2024 14:58
@GPUtester
Copy link

🎉 This PR is included in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improves an existing functionality non-breaking Introduces a non-breaking change released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants