Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Create a wheel/sdist with the baseline images#17557
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Create a wheel/sdist with the baseline images#17557
Uh oh!
There was an error while loading.Please reload this page.
Conversation
1550588 to4b46e1cCompare738ad0f toc25730fCompareSidharthBansal commentedJun 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Description about the commits: License added to mpl-baseline-only-package Created license for the mpl-baseline images. It is same License as /LICENSE/LICENSE sub-wheels/matplotlib-baseline-images/MANIFEST.in added Added Manifest to include the baseline images folder, setup, readme, license, and setup.cfg.template for the package Modified setup.cfg.template i deleted the instructions for disabling the baseline images to be installed implicitly as it is a separate package after mergingthis pr. Moved mpl_baseline_images and mpl_toolkit_baseline_images to baseline_images folder Also included mpl-toolkit_baseline_images in the package Modified links for the changed location of baseline images Changed links in the decorators for the image comparisons and in the tools/triage_tests.py Added Setup.py changes for the baseline images package Fetched the version and then strip the version as it contains 'version_number+pep_encoding'. So, used [0] of the list after split('+'). Added Readme for the baseline images package Finally added the basic documentation in Readme mpl-baseline-images package uploaded to test pypi ->https://test.pypi.org/project/matplotlib.baseline-images/3.2.1/ Thanks for the help. Kindly review@anntzer and suggest the changes. |
anntzer commentedJun 15, 2020
Sorry for the delay in reviewing. Unfortunately, neither the wheel nor the sdist seem correct. You can check this by unpacking the whl and the tar.gz that you built and uploaded to testpypi: the whl contains only metadata and no baseline images and the tar.gz contains everything, not just the baseline images. Do you understand the problem? An additional point is that decorators.py should not look up the baseline images relative to itself (because you can imagine that this won't work if e.g. matplotlib is directly installed and the baseline images are editably installed, or vice-versa). Although your modification could be kept as a fallback (in case matplotlib is editably installed and the baseline images not installed at all, but happen to be findable next to the rest of the library because that's how the git repository is laid out), the correct fix should be to instead e.g. Finally, changes to setup.cfg should be accompanied with the corresponding changes in the toplevel setupext.py. |
2605180 to46d5befCompareSidharthBansal commentedJun 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I realized that sample data doesn't need any changes. Only test data/baseline images need changes. So, pushedcommit for deletion of related feature from setupext.py |
anntzer commentedJun 16, 2020
Agreed that sample_data should be left in-place. |
SidharthBansal commentedJun 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Changed path in decorators in thiscommit |
5e56285 to6238066CompareSidharthBansal commentedJun 18, 2020
A developer can check the sdist and wheel generated for the mpl package by running For the baseline-images package, kindly check by |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
6238066 to8402af5CompareSidharthBansal commentedJun 19, 2020
Rebased |
ce0642e tof325d87Comparec726906 to85bb3ebCompare46e6602 toddbdf89Comparetacaswell commentedJul 1, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
You can put local paths into a requirements.txt file like so:https://pip.pypa.io/en/stable/reference/pip_install/#requirements-file-format ✔ 22:49:56 $ git diffdiff --git a/requirements/testing/travis_all.txt b/requirements/testing/travis_all.txtindex 3f42a603f6..040389e207 100644--- a/requirements/testing/travis_all.txt+++ b/requirements/testing/travis_all.txt@@ -8,3 +8,4 @@ pytest-timeout pytest-xdist python-dateutil tornado+-e sub-wheels/matplotlib-baseline-imagesdiff --git a/sub-wheels/matplotlib-baseline-images/setup.py b/sub-wheels/matplotlib-baseline-images/setup.pyindex ff4e1e4f95..67a3607d29 100644--- a/sub-wheels/matplotlib-baseline-images/setup.py+++ b/sub-wheels/matplotlib-baseline-images/setup.py@@ -17,5 +17,5 @@ setup( package_dir={"": "lib"}, packages=find_packages("lib"), include_package_data=True,- install_requires=["matplotlib=={}".format(__version__)],+ # install_requires=["matplotlib=={}".format(__version__)], ) which means instead of having to document "please go into the sub-package and install it" we can have people do I took out the version checking in the inner setup.py because it was failing for me due to a dirty git tree, I suspect a bit more thought needs to go into how to make that the right level of strictness. |
tacaswell commentedJul 1, 2020
changing the requirement file would also mean the changes to all of the ci configs could be reverted (as it would be done in a central place). |
c7dc022 to2b0e03aCompare| """ | ||
| module_path=Path(sys.modules[func.__module__].__file__) | ||
| baseline_dir=module_path.parent/"baseline_images"/module_path.stem | ||
| iffunc.__module__.startswith("matplotlib."): |
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.
Maybe not on this pass, but we should think about how packages can what package carries their test images.
Maybe entrypoints?
| package_dir={"":"lib"}, | ||
| packages=find_packages("lib"), | ||
| include_package_data=True, | ||
| # install_requires=["matplotlib=={}".format(__version__)], |
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.
My heavy-handed approach in my suggestion is probably too much, is this something that will be handled in a follow on PR?
tacaswell commentedJul 1, 2020
This seems to all be technically correct I have a couple of questions: For this PR:
I don't want to hold the PR for final answers, but just to make sure we have not painted ourselves in to a corner. I assume the questions of the human developer and CI workflows are being addressed in#17793 ? |
SidharthBansal commentedJul 2, 2020
We haven't thought about the entrypoints yet. The version of mpl and mpl_baseline_images package will be handled in the followup PRs I guess. |
fa37ce7 to0086a0eCompareanntzer commentedJul 2, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
re: entrypoints: I think a much simpler solution would be to make the location of the baseline images configurable, e.g. re: versioning: I think the right way is to just declare baselines as immutable (if you want to change an image just rename it, e.g. from "foo.png" to "foo.1.png" to "foo.2.png" etc.) This doesn't really help re: FreeType version, but I guess that is a separate question? Right now (especially for the baseline wheel/sdist) we still have a canonical FT version, I guess later we can e.g. add a separate subdirectory for alternative FT versions? PS: looks like upgrading setuptools fixed the azure build? |
0086a0e to957fb12CompareSidharthBansal commentedJul 3, 2020
Yes |
SidharthBansal commentedAug 18, 2020
Moved to#17793 |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
Uploading on test.pypi
.flake8to ignore theimporterrors in thelib/matplotlib/tests/__init__.pyand 'lib/mpl_toolkits/tests/_init.py`In case of running the tests/modifying the tests containing the baseline-images, the developer needs to install the
matplotlib_baseline_imagespackage.PR Checklist