-
Notifications
You must be signed in to change notification settings - Fork 13
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
Conversation
There was a problem hiding this 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 onrapids-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.
Thanks for considering it.
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 If it's
|
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 |
Thanks.
If you add an It does look like I missed one public repo...
I did that (with the 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
Can we please not couple that to this change, especially since (as far as I know) it's not currently being actively worked on? |
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 I'm going to merge this as-is. If you hear from anyone that this broke them, |
# [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))
🎉 This PR is included in version 1.14.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
, andNVIDIA
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 thatsemantic-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 onrapids-dependency-file-generator
:https://github.com/rapidsai/rapids-build-backend/blob/4e6969363bbddf8b4e94cf354077319337b5c432/pyproject.toml#L17