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

feat(cli): remove --file_key and --prepend-channels arguments #100

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

jameslamb
Copy link
Member

@jameslamb jameslamb commented Jun 17, 2024

Fixes #89

Removes the arguments --file_key and --prepend-channels.

Those have been deprecated since #71.

Notes for Reviewers

I've removed all remaining uses of them across the rapidsai, nv-morpheus, and NVIDIA GitHub organizations. See the PRs linked to #89.

Shouldn't this be a breaking change (and a 1.0 to 2.0 bump)?

This is a breaking change to the CLI but not to the library. And because so many uses of the deprecated pattern have already been removed across NVIDIA repos, there should be minimal cases where even the CLI breaks.

I'm proposing committing with a message like feat:... so that semantic-release will publish this as v1.14.0, not v2.0.0.

I'm proposing that this cause a minor version bump so that we don't have to go update rapids-build-backend's constraint on rapids-dependency-file-generator:

https://github.com/rapidsai/rapids-build-backend/blob/4e6969363bbddf8b4e94cf354077319337b5c432/pyproject.toml#L17

@jameslamb jameslamb added breaking Introduces a breaking change improvement Improves an existing functionality labels Jun 17, 2024
@jameslamb jameslamb changed the title WIP: feat(cli): remove --file_key and --prepend-channels arguments feat(cli): remove --file_key and --prepend-channels arguments Jul 1, 2024
@jameslamb jameslamb marked this pull request as ready for review July 1, 2024 23:14
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.

I am thinking about the slim possibility that someone outside of RAPIDS is using this project and would have their project broken by this removal. Weighing that against:

I'm proposing that this cause a minor version bump so that we don't have to go update rapids-build-backend's constraint on rapids-dependency-file-generator:

Updating the constraint really doesn't seem like a big deal to me, but I do understand that there is a very slim chance of this change causing a problem for anyone outside of RAPIDS. I am slightly in favor of bumping to a major version, but I won't push the matter.

@jameslamb
Copy link
Member Author

Thanks for considering it.

Updating the constraint really doesn't seem like a big deal to me

I personally think it's not worth it the effort. But I'm happy to let @bdice be the tie-breaker on whether this is 1.4.0 or 2.0.0.

If it's 2.0.0, we'll have to follow it up with these additional changes:

@KyleFromNVIDIA
Copy link
Contributor

Weren't we also considering adding the --stdout argument from #48 before a 2.0 release, since that would be a breaking change? Cc: @trxcllnt

@bdice
Copy link
Contributor

bdice commented Jul 2, 2024

I do not know of any consumers of rapids-dependency-file-generator outside of RAPIDS "core" + a few other NVIDIA repositories that are RAPIDS-adjacent. However it looks like we have several private repos where breaking the CLI without a major bump might break CI. I like being expedient where possible but I would check over this list and make sure you have investigated the possibility of breaking CI for these repos: https://github.com/search?q=org%3Arapidsai+--file_key&type=code

@jameslamb
Copy link
Member Author

jameslamb commented Jul 2, 2024

Thanks.

it looks like we have several private repos where breaking the CLI without a major bump might break CI

If you add an AND NOT is:archived filter (this search link), the only remaining private repo that would be broken by this is the *-cuml one. @dantegd told me that's not being actively maintained right now, and that it's safe to ignore for RAPIDS-wide changes until that changes. I can share a link to our conversation with you internally.

It does look like I missed one public repo... cugraph-gnn, probably because that repo is so new. I've put up rapidsai/cugraph-gnn#5.

I would check over this list and make sure you have investigated the possibility of breaking CI for these repos

I did that (with the AND NOT is:archived filter) for the rapidsai, NVIDIA, and nv-morpheus organizations. I'm confident that the only remaining place in those GitHub organizations that'd be broken by publishing this as 1.4.0 is cugraph-gnn.

If you still feel after reading that that it'd be better to make this 2.0, I'll change the title of this so that it'd be published as 2.0 and then go make the ci-imgs and rapids-build-backend changes after this is published.

Weren't we also considering adding the --stdout argument from #48 before a 2.0 release

Can we please not couple that to this change, especially since (as far as I know) it's not currently being actively worked on?

@jameslamb
Copy link
Member Author

Changes have been merged such that CI won't break in the repos mentioned in #100 (comment).

rapidsai/cugraph-gnn#5 hasn't been merged yet, but there also isn't CI there yet... so merging this PR as a 1.x release couldn't break that repo's CI.

@bdice talked about this offline, and he agreed that with those cases fixed, he'd be ok with doing this as a 1.x release.

And since it'll be a 1.x release, the conversation about whether or not to wait for --stdout to be added before cutting a 2.0 release doesn't have to be resolved here.

I'm going to merge this as-is. If you hear from anyone that this broke them, @ me and I'll help them fix it.

@jameslamb jameslamb merged commit 1f20a3e into rapidsai:main Jul 3, 2024
4 of 8 checks passed
@jameslamb jameslamb deleted the remove-deprecated branch July 3, 2024 14:10
GPUtester pushed a commit that referenced this pull request Jul 3, 2024
# [1.14.0](v1.13.11...v1.14.0) (2024-07-03)

### Features

* **cli:** remove --file_key and --prepend-channels arguments ([#100](#100)) ([1f20a3e](1f20a3e))
@GPUtester
Copy link

🎉 This PR is included in version 1.14.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
breaking Introduces a breaking change improvement Improves an existing functionality released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove "--file_key" and "--prepend_channels"
4 participants