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

Allow testing of earliest/latest dependencies. #228

Merged
merged 6 commits into from
Aug 14, 2024

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jul 11, 2024

This PR allows the testing of earliest/latest dependencies. This is meant to be interpreted by each repository as it wishes, with CI scripts and rapids-dependency-file-generator to mediate which dependency versions are selected. For example, cuDF may wish to test early versions of numpy and pandas, while cuGraph may wish to test early versions of networkx, etc.

This is an experimental PR to prompt discussion / further work with @seberg.

@@ -108,15 +108,16 @@ jobs:

echo "MATRIX=${MATRIX}" | tee --append "${GITHUB_OUTPUT}"
test:
name: ${{ matrix.CUDA_VER }}, ${{ matrix.PY_VER }}, ${{ matrix.ARCH }}, ${{ matrix.LINUX_VER }}, ${{ matrix.gpu }}
name: ${{ matrix.CUDA_VER }}, ${{ matrix.PY_VER }}, ${{ matrix.ARCH }}, ${{ matrix.LINUX_VER }}, ${{ matrix.GPU }}, ${{ matrix.DRIVER }}-driver, ${{ matrix.DEPENDENCIES }}-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

These name changes show up as:
screenshot_2024-07-16_at_16 34 22
(Maybe a bit long, but fits just about)

@seberg seberg marked this pull request as ready for review July 17, 2024 07:00
@seberg seberg requested a review from a team as a code owner July 17, 2024 07:00
@seberg seberg requested review from msarahan and removed request for a team July 17, 2024 07:00
@vyasr
Copy link
Contributor

vyasr commented Jul 24, 2024

We should definitely proceed here because we need to do it sooner than we'll be able to switch to uv, but JFYI in case it's useful for the future, uv supports using the oldest instead of newest compatible versions via a flag. It won't be as surgical as what you are doing here though.

@seberg
Copy link
Contributor

seberg commented Jul 25, 2024

Rebased. Do we need to discuss if the pattern in the RMM PR pattern is nice enough to be pushed to most RAPIDS repos?

Using uv to automatically find the oldest versions sounds great, though. Not sure the precision here has much up-sides in practice. (Even if uv ends up pip only, testing this only in the wheels is maybe good enough in practice.).

@vyasr
Copy link
Contributor

vyasr commented Jul 25, 2024

TBH if we could use uv today, I might advocate just doing that because I agree that the precision may not have much upside other than restricting the complexity of your test space (it may, in fact, have downsides as a result). The problem is that we can't use uv quite yet as discussed in rapidsai/build-planning#86 (due to rapidsai/build-planning#85).

@seberg
Copy link
Contributor

seberg commented Jul 26, 2024

I guess the question is then if we should wait for uv here if there is a clear ETA. That would avoid back and forth on the dependencies.yaml (although I don't think it is terrible churn).

One thing wI could do is pull out the conda change, since I guess with uv we would probably limit this to just pip for simplicity. (right now conda is nicer on the workflow side, but it seems tricky)

@seberg seberg changed the base branch from branch-24.08 to branch-24.10 July 31, 2024 11:47
@seberg
Copy link
Contributor

seberg commented Jul 31, 2024

Changes this to 24.10. Should we go head with this, or redesign/wait for uv?

Copy link
Contributor

@msarahan msarahan left a comment

Choose a reason for hiding this comment

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

I think we should proceed now with this. It is not much churn if/when we use uv to achieve this purpose.

- { ARCH: 'amd64', PY_VER: '3.10', CUDA_VER: '11.8.0', LINUX_VER: 'ubuntu22.04', GPU: 'v100', DRIVER: 'latest' }
- { ARCH: 'amd64', PY_VER: '3.10', CUDA_VER: '12.0.1', LINUX_VER: 'rockylinux8', GPU: 'v100', DRIVER: 'latest' }
- { ARCH: 'amd64', PY_VER: '3.11', CUDA_VER: '12.5.1', LINUX_VER: 'ubuntu22.04', GPU: 'v100', DRIVER: 'latest' }
- { ARCH: 'amd64', PY_VER: '3.9', CUDA_VER: '11.4.3', LINUX_VER: 'rockylinux8', GPU: 'v100', DRIVER: 'earliest', DEPENDENCIES: 'oldest' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the pattern for DRIVER is already 'earliest' and 'latest', can we keep that pattern for DEPENDENCIES also? In other words, just limit the values that a human has to remember?

Copy link
Contributor

Choose a reason for hiding this comment

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

Had changed it after the discussion here, OTOH, also happy to change it again. I agree here it is a bit of a dissonance.

@jakirkham
Copy link
Member

What else is still needed here?

@seberg
Copy link
Contributor

seberg commented Aug 12, 2024

Can we psh this forward? I am happy to change the name, right now it seems like a toss-up to me (slightly nicer, but diverges from the other one), so I am slightly tempted to keep it as is.

@jakirkham
Copy link
Member

@msarahan what do you think?

@bdice
Copy link
Contributor Author

bdice commented Aug 13, 2024

@seberg Resolve the PR conflicts and I think we can merge this once we agree on naming. I’ll defer to you and @msarahan on that decision.

Copy link
Member

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I support this, thanks!

@msarahan msarahan merged commit 346b3b6 into branch-24.10 Aug 14, 2024
@msarahan msarahan deleted the earliest-dependencies branch August 14, 2024 15:24
rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Aug 22, 2024
Mostly identical to gh-1606, which allows the testing of oldest/latest dependencies.  What oldest/latest dependencies means exactly has to be set by each project in their `dependencies.yaml` file for the test env.

See rapidsai/shared-workflows#228 for the workflow changes.

This is part of preparing for 2.0 (which should just work, but in the end no need to include it in the same PR).

*Modifications from draft PR:*
- I renamed it to `oldest`, as suggested by Matthew.
- Noticed that the name is different between wheel and conda workflows, so modified both to include all matrix information.

(Draft, since the shared workflow should be pushed in first to revert the branch renaming probably.  Need to test that the wheel workflow is correct.)

Authors:
  - Sebastian Berg (https://github.com/seberg)
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - James Lamb (https://github.com/jameslamb)
  - https://github.com/jakirkham

URL: #1613
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants