search

rapidsai / dependency-file-generator

https://pypi.org/project/rapids-dependency-file-generator/
Apache License 2.0
15 stars 13 forks source link

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

Closed jameslamb closed 2 months ago

jameslamb commented 2 months ago

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 https://github.com/rapidsai/dependency-file-generator/issues/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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 https://github.com/rapidsai/cugraph-gnn/pull/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 https://github.com/rapidsai/dependency-file-generator/pull/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 commented 2 months ago

Changes have been merged such that CI won't break in the repos mentioned in https://github.com/rapidsai/dependency-file-generator/pull/100#issuecomment-2203962435.

https://github.com/rapidsai/cugraph-gnn/pull/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.

GPUtester commented 2 months ago

:tada: This PR is included in version 1.14.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: