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

Removes the usage of conda._vendor.toolz from constructor #525

Merged
merged 6 commits into from
Jul 26, 2022

Conversation

travishathaway
Copy link
Contributor

@travishathaway travishathaway commented Jul 7, 2022

This pull request replaces the usage of toolz in the main conda project. In an upcoming conda, release this will be completely removed.

I chose to completely copy over the code from the toolz repository into this one. It now lives in the top level toolz.py file. I also ignored the Python 2 compatibility code that was still in the conda version and instead updated based on the most recent toolz version.

It should also be noted that the get function being imported from conda._vendor.toolz was not being used and was not ported over.

See this issue for more information: conda/conda#11333

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Jul 7, 2022
@travishathaway
Copy link
Contributor Author

The test coverage in this repository is pretty low, so I would not rely on the tests passing as the best way to determine this is ready to be merged.

Does anyone know of another way we could test out the changes to make sure they haven't broken anything?

@larsoner
Copy link

larsoner commented Jul 7, 2022

Does anyone know of another way we could test out the changes to make sure they haven't broken anything?

I guess we could figure out the right commands manually every time to test any changes 😱 But this isn't sustainable, and it's also limited to one OS, etc...

The best way forward I think would be to improve the tests:

  1. One way would be via additional pytest code that targets individual functions.

  2. Another would be to adapt the code we use to build-install-test the installers on all three OSes in MNE-Python:

    https://github.com/mne-tools/mne-installers/blob/main/.github/workflows/build.yml

  3. Really the best combination would probably be some pytest fixture magic that had build-and-install (to tmpdir) capabilities, i.e., something that took what we do in mne-installers and built it into a fixture somehow. For this the closest thing I can think of is the sphinx_app_wrapper from sphinx-gallery that makes it easy to do an isolated sphinx build via sphinx_app_wrapper.create_sphinx_app:

    https://github.com/sphinx-gallery/sphinx-gallery/blob/7347cd47a8623a89919fb3fd744150a074a49431/sphinx_gallery/tests/test_gen_gallery.py#L37-L41

    We'd have to see how long this took, but if it's only a few or tens of seconds per recipe, then we can try to make a diverse minimal set of recipes to hopefully hit most code paths...

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.

As my comments suggest I'm clearly not a fan of copying code 😄

constructor/toolz.py Outdated Show resolved Hide resolved
constructor/toolz.py Outdated Show resolved Hide resolved
@travishathaway travishathaway changed the title Removes the usage of toolz from conda Removes the usage of toolz from constructor Jul 8, 2022
@travishathaway travishathaway changed the title Removes the usage of toolz from constructor Removes the usage of conda._vendor.toolz from constructor Jul 8, 2022
@jezdez jezdez requested a review from a team July 18, 2022 11:41
@kenodegard kenodegard linked an issue Jul 20, 2022 that may be closed by this pull request
2 tasks
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.

Oops! Sorry for not paying more attention and suggesting these changes during the last review. Most of these itertools use cases actually seem unnecessary and can be replaced with arg unpacking and list-comprehensions.

constructor/fcp.py Outdated Show resolved Hide resolved
constructor/fcp.py Outdated Show resolved Hide resolved
constructor/fcp.py Outdated Show resolved Hide resolved
tests/test_fcp.py Outdated Show resolved Hide resolved
constructor/fcp.py Outdated Show resolved Hide resolved
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.

Noice!

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.

Just removing two unused imports

constructor/fcp.py Outdated Show resolved Hide resolved
constructor/fcp.py Outdated Show resolved Hide resolved
travishathaway and others added 6 commits July 25, 2022 09:12
This commit attempts to replace the usage of toolz in the main conda
project. In an upcoming conda, release this will be completely removed.
Co-authored-by: Ken Odegard <kodegard@anaconda.com>
Copy link
Contributor

@beeankha beeankha left a comment

Choose a reason for hiding this comment

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

👍

@kenodegard kenodegard merged commit 10fe8e4 into main Jul 26, 2022
@kenodegard kenodegard deleted the remove-conda-vendor-toolz branch July 26, 2022 13:48
@github-actions github-actions bot added the locked [bot] locked due to inactivity label Jul 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed [bot] added once the contributor has signed the CLA locked [bot] locked due to inactivity
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove usage of conda._vendor.toolz and add dependency cytoolz instead
6 participants