Skip to content

Add PendingDeprecationWarning for vendored tqdm #12005

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Nov 1, 2022
Merged

Add PendingDeprecationWarning for vendored tqdm #12005

merged 15 commits into from
Nov 1, 2022

Conversation

ForgottenProgramme
Copy link
Contributor

@ForgottenProgramme ForgottenProgramme commented Oct 21, 2022 •
edited
Loading

Resolves #11432

Description

Test PR to see if removing tqdm as vendored dependency and adding it instead as a real dependency breaks anything.
Edit: Nothing breaks.

In this PR we add DeprecationWarning for vendored tqdm, before deprecating it in the next release.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@ForgottenProgramme ForgottenProgramme requested a review from a team as a code owner October 21, 2022 11:18
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Oct 21, 2022
dholth
dholth requested changes Oct 21, 2022
View reviewed changes
Copy link
Contributor

@dholth dholth left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, except the lower version should be much less (possibly as low as tqdm >= 4). We only have 4.64.0 in defaults: https://metayaml-conda.fly.dev/metayaml/newest_by_platform?name=tqdm

I would change this to "tdmq >=4" since defaults only has 4.64.0 and there is no special reason to require the newest version. Here's how other things in defaults have declared a tqdm dependency: https://metayaml-conda.fly.dev/metayaml/reverse_dependencies?name=tqdm

kenodegard
kenodegard requested changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Beware that we have had a lot of issues removing vendored code in favor of a dependency in the same release.

I suggest we only add tqdm as a dependency in the next release and mark the vendored code as pending deprecation per CEP8.

cc @jezdez

@jezdez
Copy link
Member

jezdez commented Oct 24, 2022

Beware that we have had a lot of issues removing vendored code in favor of a dependency in the same release.

I suggest we only add tqdm as a dependency in the next release and mark the vendored code as pending deprecation per CEP8.

cc @jezdez

Yeah agreed, the situation around toolz has been not great for us or the community and using a multi-step approach to cleaning out the vendored code is sensible. Thanks for raising this @kenodegard, you're absolutely right.

Mahe Iram Khan added 2 commits October 28, 2022 10:43
Revert "remove tqdm dir from _vendor"
a80900a 
Revert the commit removing the tqdm directory, add deprecation warning for tqdm instead
add PendingDeprecationWarning to tqdm
e56ff03
travishathaway
travishathaway previously approved these changes Oct 28, 2022
View reviewed changes
jezdez
jezdez previously approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

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

LGTM, other than a bit of wording in the deprecation message. Thank you @ForgottenProgramme!

kenodegard
kenodegard requested changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@kenodegard kenodegard left a comment

Choose a reason for hiding this comment

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

Needs a news snippet

@ForgottenProgramme @jezdez
Update conda/_vendor/tqdm/__init__.py
fc96aef 
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@ForgottenProgramme ForgottenProgramme changed the title Change 'tqdm' from vendored dependency to real dependency for conda Add PendingDeprecationWarning for tqdm Oct 28, 2022
dholth
dholth previously approved these changes Oct 28, 2022
View reviewed changes
@ForgottenProgramme ForgottenProgramme changed the title Add PendingDeprecationWarning for tqdm Add PendingDeprecationWarning for vendored tqdm Oct 28, 2022
beeankha
beeankha previously approved these changes Oct 28, 2022
View reviewed changes
jezdez
jezdez requested changes Oct 28, 2022
View reviewed changes
@ForgottenProgramme @jezdez
Make warning message more specific
d03d7ca 
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@ForgottenProgramme ForgottenProgramme dismissed stale reviews from beeankha and dholth via d03d7ca October 28, 2022 14:51
kenodegard
kenodegard previously approved these changes Oct 28, 2022
View reviewed changes
jezdez
jezdez previously approved these changes Oct 28, 2022
View reviewed changes
@jezdez jezdez enabled auto-merge (squash) October 28, 2022 15:09
beeankha
beeankha previously approved these changes Oct 31, 2022
View reviewed changes
@jezdez jezdez dismissed stale reviews from beeankha, kenodegard, and themself via ab5176f November 1, 2022 08:27
@jezdez jezdez merged commit 8cc15f1 into conda:main Nov 1, 2022
jeskowagner pushed a commit to jeskowagner/conda that referenced this pull request Nov 9, 2022
@ForgottenProgramme @jezdez @jeskowagner
Directly depend on tqdm and raise PendingDeprecationWarning for vendo…
510fd46 
…red tqdm (conda#12005)

Resolves conda#11432

Co-authored-by: Mahe Iram Khan <maheiramkhan@Mahes-MacBook-Pro.fritz.box>
Co-authored-by: Mahe Iram Khan <maheiramkhan@Mahes-MBP.fritz.box>
Co-authored-by: Jannis Leidel <jannis@leidel.info>
@jonashaag
Copy link
Contributor

jonashaag commented Dec 12, 2022 •
edited
Loading

Could it be that this broke the recipe in the repo? We test Conda nightly with Mamba and a few days ago the build broke: https://github.com/mamba-org/mamba/actions/workflows/conda_nightly.yml Although I'm not quite sure why the build broke on this particular day, adding tqdm to the host: requirements seems to fix it. If this is the case, does the Conda CI not test the recipe?

@jonashaag jonashaag mentioned this pull request Dec 12, 2022
Closed
@jonashaag
Copy link
Contributor

@jezdez @ForgottenProgramme

@kenodegard
Copy link
Contributor

@jonashaag I wonder if this is potentially related to the need to introduce the isolated mode in conda-build:

If this is the case, does the Conda CI not test the recipe?

What do you mean?

Let's discuss further in a new issue for better visibility

@jonashaag jonashaag mentioned this pull request Dec 19, 2022
Closed
@jakirkham jakirkham mentioned this pull request Jan 20, 2023
Closed
2 tasks
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Dec 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jezdez jezdez jezdez approved these changes

@dholth dholth dholth left review comments

@travishathaway travishathaway travishathaway left review comments

@kenodegard kenodegard kenodegard left review comments

@beeankha beeankha beeankha left review comments

Assignees
No one assigned
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
🧭 Planning
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove vendored tqdm and add it as a real dependency
8 participants
@ForgottenProgramme @jezdez @jonashaag @kenodegard @dholth @travishathaway @beeankha @conda-bot