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

[v0.11] Vendor the Version package, use it instead of distutils #2766

Merged
merged 7 commits into from
Apr 3, 2022

Conversation

Stannislav
Copy link

@Stannislav Stannislav commented Mar 20, 2022

Fixes #2724

Cherry-picked a0f7bf8 / #2659 as discussed in #2724

@Stannislav Stannislav changed the title Vendor the Version package, use it instead of distutils [v0.11] Vendor the Version package, use it instead of distutils Mar 20, 2022
@mwaskom
Copy link
Owner

mwaskom commented Mar 20, 2022

Thanks @Stannislav! Looks like CI is not set up to run on PRs against the v0.11 branch; would you mind adding that as part of this PR?

@codecov
Copy link

codecov bot commented Mar 20, 2022

Codecov Report

Merging #2766 (65d91cb) into v0.11 (e8a83c8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            v0.11    #2766      +/-   ##
==========================================
+ Coverage   96.98%   97.03%   +0.04%     
==========================================
  Files          17       17              
  Lines        6233     6233              
==========================================
+ Hits         6045     6048       +3     
+ Misses        188      185       -3     
Impacted Files Coverage Δ
seaborn/_core.py 98.13% <100.00%> (ø)
seaborn/_statistics.py 99.02% <100.00%> (ø)
seaborn/axisgrid.py 95.99% <100.00%> (ø)
seaborn/categorical.py 98.00% <100.00%> (ø)
seaborn/rcmod.py 100.00% <100.00%> (ø)
seaborn/_testing.py 98.11% <0.00%> (+5.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8a83c8...65d91cb. Read the comment docs.

@Stannislav
Copy link
Author

@mwaskom has the v0.11 branch been tested recently? Not sure some failing tests have to do with the changes in this PR. I'll try and look into the details tonight.

@mwaskom
Copy link
Owner

mwaskom commented Mar 21, 2022

@mwaskom has the v0.11 branch been tested recently? Not sure some failing tests have to do with the changes in this PR

no, I wasn’t expecting to do an 0.11.3 so it’s been dormant for a while. I don’t think the test failures are caused by this pr; matplotlib broke some tests with their 3.5 release (by changing an API that only the tests hit); they’ve also added some deprecation warnings that get a lot of false alarms and add noise.

I’m less sure what’s happening with the failing setup in the doc build, but also doubt this PR caused it.

@Stannislav
Copy link
Author

@mwaskom the docs build is failling because of sphinx_bootstrap_theme==0.7.1 in docs/requirements.txt. The problem is that this package is using setup(use_2to3=True, ...) in setup.py, which was removed in setuptools>=58. To reproduce

pip install 'setuptools>=58'
pip install 'sphinx_bootstrap_theme==0.7.1'

The next higher version is sphinx_bootstrap_theme==0.8.0 and it fixes the error. Any reason not to bump? I see it was updated on master in #2704.

@mwaskom
Copy link
Owner

mwaskom commented Mar 21, 2022

The next higher version is sphinx_bootstrap_theme==0.8.0 and it fixes the error. Any reason not to bump? I see it was updated on master in #2704.

Ah yeah, that sounds right.

Stanislav Schmidt added 2 commits March 28, 2022 13:41
* Update boxplot tests for mpl3.5 compatability

* Update kdeplot tests for mpl3.5 compatability

* Update legend tests for mpl3.5 compatability

* Pin docutils to avoid buggy interaction with sphinx
@Stannislav
Copy link
Author

@mwaskom I bumped sphinx-bootstrap-theme and cherry-picked #2690 to try and fix the tests after matplotlib 3.5 upgrade. Is it possible to re-trigger the CI to try and test?

@mwaskom
Copy link
Owner

mwaskom commented Mar 31, 2022

Thanks @Stannislav sorry this is turning into such a headache for you. I'm not sure why new contributors need every PR push to be approved for CI (rather than just the first one).

@Stannislav
Copy link
Author

Hey @mwaskom no worries. Maybe the approval is needed because it's a forked branch.

Anyway, a small change adopted from master (from #2413) fixed the test locally, so we can try to retest.

@mwaskom
Copy link
Owner

mwaskom commented Apr 3, 2022

Maybe the approval is needed because it's a forked branch.

Yeah — I think it's to stop people from opening a PR that mines crypto in CI, so it makes sense to require approval for the first run, but I wish there were a checkbox to say "run CI on subsequent pushes for this PR".

In any case, everything looks good here now — thanks!

@mwaskom mwaskom merged commit 10fc8d7 into mwaskom:v0.11 Apr 3, 2022
@Stannislav Stannislav deleted the v0.11 branch April 3, 2022 15:28
@EwoutH
Copy link
Contributor

EwoutH commented May 30, 2022

Could a new release on the v0.11.x branch be tagged, which includes this change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants