jaraco / pytest-perf

MIT License
3 stars 2 forks source link

Allow installing from an existing wheel #3

Closed abravalheri closed 1 year ago

abravalheri commented 2 years ago

Recently when playing around with the idea proposed in https://github.com/pypa/setuptools/pull/3015#pullrequestreview-847184271, I noticed that pytest-perf requires the package to be installed from ..

However, as proposed in the linked comment, not all projects will perform tests by installing from .. Some will try to use a pre-build distribution.

Would it be possible for pytest-perf to allow some sort of configuration for that?

In this PR I am suggesting something very simple based on an environment var: PYTEST_PERF_WHEEL_TARGET (that allows only wheels since their name is normalized and easy to extract from the file path).

Please let me know if you find this interesting. I don't know how to add unit tests, but this patch seems to work as show by these CI logs that use this configuration.

jaraco commented 2 years ago

See 171164e for how I might approach making the target configurable in BenchmarkRunner.

abravalheri commented 2 years ago

Thank you very much for the code review @jaraco. I implemented a few changes (I am afraid the PR grew a bit 😅 sorry for that).

I tried to use 171164e, but unfortunately pep517 seems to have problems with distribution files, so I tried a different approach, as described in https://github.com/jaraco/pytest-perf/pull/3#discussion_r790304617. Hopefully that is better?

Please feel free to revert any of the commits. I just tested on linux and things seem to be fine, but I would rely on the CI to see if there is something wrong happening on windows or PyPy

jaraco commented 2 years ago

I see you use the term "distribution files". When you say that, do you mean to support binary distributions (wheels), source distributions (sdist), both, or something else?

abravalheri commented 2 years ago

I see you use the term "distribution files". When you say that, do you mean to support binary distributions (wheels), source distributions (sdist), both, or something else?

I recently started using the term "distribution files" (or "distribution archives") to refer to both/any wheel and sdist (because "package" already has a meaning to the Python interpreter and one distribution file can have multiple packages).

The original implementation of the PR was focusing on wheels because these files have a normalised name and therefore it was straight forward to derive the distribution name. But without relying on that method, there should be no difference in handling wheels or sdists.

abravalheri commented 2 years ago

I noticed some problems in the CI for Windows (apparently TemporaryDirectory has problems with removing .git directories 😓) so I added some workaround for that on 188c654. Please let me know if that is OK.

jaraco commented 2 years ago

You've done a lot of work here, and I really appreciate it.

There's a lot going on and I've made a number of critiques/requests above, but as I've mentioned in some of the comments, I think I'd like to step back and revisit the implementation and try to separate out the two concerns (custom target vs. custom baseline).

I realize it may add delays, but I'd like to do some more experimentation and exploration based on the work above and see if I can come up with something that I'm a bit more comfortable with. Do you have any sensitive timelines for this change?

abravalheri commented 2 years ago

Hi @jaraco, I just would like to thank you for the review and say that I still haven't had the time to go through your comments and implement the related changes.

Regarding your question:

Do you have any sensitive timelines for this change?

I don't have any sensitive timeline, I was just experimenting with ways of testing setuptools without usedevelop=True and found some limitations in pytest-perf so I decided to start this PR.

I hope to get back to it soon 😄

jaraco commented 1 year ago

I'm going to close this PR for now, but without prejudice. Feel free to revive conversation here and I can re-open if appropriate, or use the feedback above to create new PRs for the separate concerns.