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

Always upload on branch/nightly builds #1177

Merged
merged 2 commits into from
May 25, 2023

Conversation

raydouglass
Copy link
Member

Since the build.yaml workflow only runs on branch pushes, tag pushes, or nightly calls, it should always upload the wheel to PyPI like it does for conda packages.

This will fix the missing release uploads like this: https://github.com/rapidsai/dask-cuda/actions/runs/4678841210/jobs/8288889977

@raydouglass raydouglass requested a review from a team as a code owner May 24, 2023 18:30
@raydouglass raydouglass self-assigned this May 24, 2023
@raydouglass raydouglass added bug Something isn't working non-breaking Non-breaking change labels May 24, 2023
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

this change will cause releases to occur when PRs are merged now.

because of that, we should probably also use the skip-existing parameter below:

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I'm fine with this change, just want to make sure that the consequences are clear:

  • If there are no PRs on a given day, then we get a true nightly at the end of the day.
  • If there is exactly one PR on a given day, the "nightly" for that day will be the package uploaded when that PR is merged.
  • If there are multiple PRs, then the pushed package will be from the first merged PR. All subsequent builds will simply skip uploading.

It's an acceptable tradeoff, it just means that sometimes the latest nightly won't actually contain the latest changes.

Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

approving but also CC'ing @pentschev for some feedback based on Vyas's comment

@pentschev
Copy link
Member

If there are multiple PRs, then the pushed package will be from the first merged PR. All subsequent builds will simply skip uploading.
It's an acceptable tradeoff, it just means that sometimes the latest nightly won't actually contain the latest changes.

I must say this is a bit weird, essentially the newest nightly will not contain the latest changes. I don't know whether anyone actually uses Dask-CUDA PyPI nightly builds but our own CI. If you think this is an acceptable tradeoff I'm ok with that, but what this is saying is that we may need to wait up to ~48h for changes to show up in CI on those days (assuming two PRs are merged shortly after the latest nightly was generated), which could mean it will be blocked for up to 48h. Dask-CUDA has low traffic so this is probably not going to be an occurrence more than a few days a year, but just wanted to point it out.

@ajschmidt8
Copy link
Member

Is there an alternative versioning scheme we can adopt for nightlies to fix this weirdness?

e.g. one of the following, where GIT_DESCRIBE_NUMBER is the number of commits since the most recent tag:

23.6.0a230524<GIT_DESCRIBE_NUMBER>
23.6.0a230524.dev<GIT_DESCRIBE_NUMBER>

both of these are PEP 440 compliant.

and using GIT_DESCRIBE_NUMBER would ensure that each commit gets released.

@pentschev
Copy link
Member

Is there an alternative versioning scheme we can adopt for nightlies to fix this weirdness?

I have no preference here, anything would be ok. I'm also ok with leaving it like this for now and revisit should this become important. I'll leave it up to @vyasr who is more knowledgeable what would be better here or if he has any preference, otherwise I'm ok merging this as is for now.

@ajschmidt8
Copy link
Member

Ok. I assume we can merge this now then and revisit this conversation in the future if necessary.

I'll defer to Ray to merge since it's his PR.

@raydouglass
Copy link
Member Author

/merge

@rapids-bot rapids-bot bot merged commit 1681510 into rapidsai:branch-23.06 May 25, 2023
@raydouglass raydouglass deleted the release-upload branch May 25, 2023 13:20
@jakirkham
Copy link
Member

Thanks all! 🙏

@vyasr
Copy link
Contributor

vyasr commented May 31, 2023

Sorry was out of town when the discussion was being concluded. Yes, there are alternative version schemes that we can adopt to fix the problem. However, we would probably want to apply this consistently across all of RAPIDS. Furthermore, it would affect our dependency specs because of the way pip installs nightlies (which is also why the rest of RAPIDS doesn't currently use proper nightly versions). Happy to revisit this discussion in the future though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants