Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Conversation

SidharthBansal
Copy link
Contributor

@SidharthBansalSidharthBansal commentedJun 3, 2020
edited
Loading

PR Summary

  • Create a wheel/sdist with just the baseline images; upload it to testpypi (so that one can do pip install mpl-baseline-images); don’t bother removing baseline images from the main repo yet.
  • Changes in the MANIFEST for the matplotlib package is required for not including the matplotlib_baseline_images package.
    Uploading on test.pypi
  • Created tests/test_data. Tests/test_data contains the images used in the tests and we don't want need in the baseline_images package.
  • Symlink baseline images out.
  • Changes in the travis, appvoyer and azure pipeline to install matplotlib_baseline_images package after matplotlib package is installed
  • Changes in the.flake8 to ignore theimport errors in thelib/matplotlib/tests/__init__.py and 'lib/mpl_toolkits/tests/_init.py`

In case of running the tests/modifying the tests containing the baseline-images, the developer needs to install thematplotlib_baseline_images package.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@SidharthBansalSidharthBansal marked this pull request as draftJune 3, 2020 14:08
@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch from1550588 to4b46e1cCompareJune 5, 2020 17:57
@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 5 times, most recently from738ad0f toc25730fCompareJune 12, 2020 16:03
@SidharthBansal
Copy link
ContributorAuthor

SidharthBansal commentedJun 12, 2020
edited
Loading

Description about the commits:
Moved Baseline images to a sub-wheel/matplotlib-baseline-images commit moves the baseline images out of the lib folder to the newly created package directory matplotlib-baseline images

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('+').
Then used the setup to initialise the values.

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
Copy link
Contributor

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.import matplotlib_baseline_images and look up the images relative tomatplotlib_baseline_images.__file__ (assuming you decide to create an emptymatplotlib_baseline_images/__init__.py after all just for the purposes of locating things relative to it), or to use something likeimportlib.resources.path (https://docs.python.org/3/library/importlib.html#importlib.resources.path) orimportlib.metadata.files (https://docs.python.org/3/library/importlib.metadata.html#files) to locate the files of that package.

Finally, changes to setup.cfg should be accompanied with the corresponding changes in the toplevel setupext.py.

SidharthBansal reacted with thumbs up emoji

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch from2605180 to46d5befCompareJune 16, 2020 13:01
@SidharthBansal
Copy link
ContributorAuthor

SidharthBansal commentedJun 16, 2020
edited
Loading

changes to setup.cfg should be accompanied with the corresponding changes in the toplevel setupext.py.

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
Copy link
Contributor

Agreed that sample_data should be left in-place.

SidharthBansal reacted with thumbs up emoji

@SidharthBansal
Copy link
ContributorAuthor

SidharthBansal commentedJun 16, 2020
edited
Loading

Changed path in decorators in thiscommit

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 6 times, most recently from5e56285 to6238066CompareJune 18, 2020 13:21
@SidharthBansal
Copy link
ContributorAuthor

A developer can check the sdist and wheel generated for the mpl package by runningpython3 setup.py sdist bdist_wheel from the terminal from thedist folder. It will not havesub-wheel directory

For the baseline-images package, kindly check bycd sub-wheels/matplotlib-baseline-images and thenpython3 setup.py sdist bdist_wheel from thesib-wheels/matplotlib-baseline-images/dist folder. It will only contain baseline images directory related files and folders.

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch from6238066 to8402af5CompareJune 19, 2020 16:09
@SidharthBansal
Copy link
ContributorAuthor

Rebased

@SidharthBansalSidharthBansal marked this pull request as ready for reviewJune 19, 2020 16:12
@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 4 times, most recently fromce0642e tof325d87CompareJune 22, 2020 09:14
@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 3 times, most recently fromc726906 to85bb3ebCompareJune 26, 2020 17:04
@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 3 times, most recently from46e6602 toddbdf89CompareJune 29, 2020 18:03
@tacaswell
Copy link
Member

tacaswell commentedJul 1, 2020
edited
Loading

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 dopip install -r requirements/testing/travis_all.txt (maybe renaming that file building on#15617 ?) so that to@jklymak 's point we have a very easy installation instruction installs the test images + pytest and friends.

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.

SidharthBansal reacted with thumbs up emoji

@tacaswell
Copy link
Member

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).

SidharthBansal reacted with thumbs up emoji

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 3 times, most recently fromc7dc022 to2b0e03aCompareJuly 1, 2020 14:22
@@ -482,7 +482,25 @@ def _image_directories(func):
doesn't exist.
"""
module_path = Path(sys.modules[func.__module__].__file__)
baseline_dir = module_path.parent / "baseline_images" / module_path.stem
if func.__module__.startswith("matplotlib."):
Copy link
Member

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__)],
Copy link
Member

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
Copy link
Member

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
Copy link
ContributorAuthor

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.

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch 3 times, most recently fromfa37ce7 to0086a0eCompareJuly 2, 2020 18:15
@anntzer
Copy link
Contributor

anntzer commentedJul 2, 2020
edited
Loading

re: entrypoints: I think a much simpler solution would be to make the location of the baseline images configurable, e.g.def image_comparison(..., *, baseline_images_package=...) and then e.g. astropy would just doimage_comparison = functool.partial(image_comparison, baseline_images_package=...) (or have global state settable viaset_baseline_images_package(...), or possibly make the configurable item a mapping of package names to baseline package names (so that different packages don't step on another). In any case I'd rather not involve more packaging tooling.

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?

@SidharthBansalSidharthBansalforce-pushed thebaseline-images-only-separate-package branch from0086a0e to957fb12CompareJuly 3, 2020 10:38
@SidharthBansal
Copy link
ContributorAuthor

Yes

@SidharthBansal
Copy link
ContributorAuthor

Moved to#17793

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

@anntzeranntzeranntzer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@SidharthBansal@anntzer@jklymak@tacaswell

[8]ページ先頭

©2009-2025 Movatter.jp