8000 gh-294: Vendor lib2to3 by corona10 · Pull Request #302 · python/pyperformance · GitHub
[go: up one dir, main page]

Skip to content

gh-294: Vendor lib2to3 #302

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

Merged
merged 6 commits into from
Jun 12, 2023
Merged

gh-294: Vendor lib2to3 #302

merged 6 commits into from
Jun 12, 2023

Conversation

corona10
Copy link
Member
@corona10 corona10 commented Jun 11, 2023
  • Use lib2to3 of CPython latest 3.12 version
  • Vendor lib2to3 except *.pickle files. (I am not sure it will affect to benchmark result)
  • Remove deprecated warning for lib2to3

closes: gh-294

@corona10
Copy link
Member Author
working_solution

@corona10 corona10 marked this pull request as ready for review June 11, 2023 07:04
@CAM-Gerlach CAM-Gerlach changed the title gh-294: Vendoring lib2to3 gh-294: Vendor lib2to3 Jun 11, 2023
Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

One significant packaging-related recommendation, as well as one other minor comment.

@corona10 corona10 requested a review from CAM-Gerlach June 11, 2023 14:35
@corona10
Copy link
Member Author

@CAM-Gerlach Thanks done!

@corona10
Copy link
Member Author

test / ubuntu-latest - pypy-3.8 (pull_request)

Not sure which is the root cause.

FAIL: test_run_and_show (test_commands.FullStackTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/pyperformance/pyperformance/pyperformance/tests/test_commands.py", line 151, in test_run_and_show
    text = self.run_pyperformance(
  File "/home/runner/work/pype
8000
rformance/pyperformance/pyperformance/tests/test_commands.py", line 76, in run_pyperformance
    self.assertEqual(ec, exitcode, repr(stdout))
AssertionError: 1 != 0 : None

@corona10 corona10 closed this Jun 11, 2023
@corona10 corona10 reopened this Jun 11, 2023
@AlexWaygood
Copy link
Member
AlexWaygood commented Jun 11, 2023

test / ubuntu-latest - pypy-3.8 (pull_request)

Not sure which is the root cause.

FAIL: test_run_and_show (test_commands.FullStackTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/pyperformance/pyperformance/pyperformance/tests/test_commands.py", line 151, in test_run_and_show
    text = self.run_pyperformance(
  File "/home/runner/work/pyperformance/pyperformance/pyperformance/tests/test_commands.py", line 76, in run_pyperformance
    self.assertEqual(ec, exitcode, repr(stdout))
AssertionError: 1 != 0 : None

The docutils benchmark seems to be pretty flaky on PyPy for some reason. I have no idea why, but I've noticed it failing on a lot of PRs and pushes recently (but not on all of them, just some of them). I've been meaning to open an issue about it, but haven't got round to it.

I'm pretty sure the failure is unrelated to this PR, anyway :)

@corona10
Copy link
Member Author

I'm pretty sure the failure is unrelated to this PR, anyway :)

Thank you Alex for checking this issue :)

Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Standard reminder: You can directly apply all the suggestions you want in one go by going to Files changed -> Clicking Add to batch on each suggestion -> When done, clicking Commit

One thing I forgot to mention—to legally include the lib2to3 code here, you'll need to include the PSF v2 license under which lib2to3 is licensed (as a module added in Python >2.2+ with no documented additional terms) to a LICENSE.txt file in the vendor directory (which will get picked up by Setuptools and Wheel automatically), and also add the appropriate license identifier and classifiers to the metadata (which I added review suggestions for). Here is the exact relevant license reproduced from the CPython LICENSE file:

License text you need to add to a `LICENSE.txt` file in `vendor`
PYTHON SOFTWARE FOUNDATION LICENSE VERSION 2
--------------------------------------------

1. This LICENSE AGREEMENT is between the Python Software Foundation
("PSF"), and the Individual or Organization ("Licensee") accessing and
otherwise using this software ("Python") in source or binary form and
its associated documentation.

2. Subject to the terms and conditions of this License Agreement, PSF hereby
grants Licensee a nonexclusive, royalty-free, world-wide license to reproduce,
analyze, test, perform and/or display publicly, prepare derivative works,
distribute, and otherwise use Python alone or in any derivative version,
provided, however, that PSF's License Agreement and PSF's notice of copyright,
i.e., "Copyright (c) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010,
2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021, 2022, 2023 Python Software Foundation;
All Rights Reserved" are retained in Python alone or in any derivative version
prepared by Licensee.

3. In the event Licensee prepares a derivative work that is based on
or incorporates Python or any part thereof, and wants to make
the derivative work available to others as provided herein, then
Licensee hereby agrees to include in any such work a brief summary of
the changes made to Python.

4. PSF is making Python available to Licensee on an "AS IS"
basis.  PSF MAKES NO REPRESENTATIONS OR WARRANTIES, EXPRESS OR
IMPLIED.  BY WAY OF EXAMPLE, BUT NOT LIMITATION, PSF MAKES NO AND
DISCLAIMS ANY REPRESENTATION OR WARRANTY OF MERCHANTABILITY OR FITNESS
FOR ANY PARTICULAR PURPOSE OR THAT THE USE OF PYTHON WILL NOT
INFRINGE ANY THIRD PARTY RIGHTS.

5. PSF SHALL NOT BE LIABLE TO LICENSEE OR ANY OTHER USERS OF PYTHON
FOR ANY INCIDENTAL, SPECIAL, OR CONSEQUENTIAL DAMAGES OR LOSS AS
A RESULT OF MODIFYING, DISTRIBUTING, OR OTHERWISE USING PYTHON,
OR ANY DERIVATIVE THEREOF, EVEN IF ADVISED OF THE POSSIBILITY THEREOF.

6. This License Agreement will automatically terminate upon a material
breach of its terms and conditions.

7. Nothing in this License Agreement shall be deemed to create any
relationship of agency, partnership, or joint venture between PSF and
Licensee.  This License Agreement does not grant permission to use PSF
trademarks or trade name in a trademark sense to endorse or promote
products or services of Licensee, or any third party.

8. By copying, installing or otherwise using Python, Licensee
agrees to be bound by the terms and conditions of this License
Agreement.

Copy link
Member

Choose a reason for hiding this comment

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

Since you mentioned wanting to avoid this hack and wanting to install it with pyproject.toml instead, I thought some more about this and left a comment below explaining how you should be able to avoid all the changes to this file and instead just add lib2to3 it as a dependency to the bm_2to3/pyproject.toml.

Copy link
Member

Choose a reason for hiding this comment

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

What he said.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since you mentioned wanting to avoid this hack and wanting to install it with pyproject.toml instead, I thought some more about this and left a comment below explaining how you should be able to avoid all the changes to this file and instead just add lib2to3 it as a dependency to the bm_2to3/pyproject.toml.

What he said.

As I commented on the discord, VCS approach is the most approximated way to handle this, but I will keep the current approach since managing the VCS URL has some issues when we want to modify the codes and the bootstrap(?) problem.

@CAM-Gerlach
Copy link
Member

@corona10 If you want to install it from the parent pyproject.toml as you wanted to do rather than part of the script, that's definitely be best as you say.

It's not really easily possible to do it truly locally, at least in a standards-conformant, non-hacky/bespoke way. You could add that subdirectory to the package finder to install it as another top-level package, but there's no way to make that conditional on a specific Python version without using a custom, backend-specific dynamic setup.py. Adding it as a dependency with the above marker allows it to be conditional on Python version and installed separately, and makes more sense, but to do that with a local path requires a non-standard, backend-bespoke mechanism (e.g. like this with hatchling, or possibly with setup.py hacks with Setuptools) and is apparently not compatible with pip and possibly other frontends (at least not yet).

However, there are several options to still approximate this, and what seems the least work and lowest friction is to specify it as a dependency using a VCS URL reference to this online repository, with the appropriate subdirectory, and git tag specified (which to test the PR prior to merging, will need to be the appropriate branch on your fork), and with a PEP 508 marker limiting it to Python 3.13+. I.e., the dependencies in bm_2to3/pyproject.toml would be:

dependencies = ["pyperf", 'lib2to3 @ https://github.com/python/pyperformance.git@2to3v3.12#subdirectory=pyperformance/data-files/benchmarks/bm_2to3/vendor ; python_version >= "3.13.0a0"']

This requires minimal extra work up front, though it does make development a bit more tedious if you ever want to make changes to lib2to3 (e.g. for compatibility with a new Python runtime version), as you'll need to push your changes to your fork, modify the repo and ref to test them, and then remember to point it back to a new git tag on the upstream before merging (if you don't specify a tag, existing versions won't be reproducible and will break if you e.g. change any part of the path, or make any incompatible changes to lib2to3 itself). I initially was going to suggest just using the tag of the pyperformance release, but it makes a lot more sense to just use a separate tag for lib2to3 as its easier and more flexible overall.

Alternatively, you could host it in its own separate repo, but I'm not sure that would really offer much benefit over just using a separate tag for lib2to3 changes, compared to the additional overhead and other costs. And/or, you could upload it as a forward-port to PyP which would simplify the spec, but otherwise add even more complexity for not much benefit (other than a little more long-term stability). So, I'd recommend the above approach assuming you want to avoid the run_benchmark,py hacks here.

@CAM-Gerlach
Copy link
Member
CAM-Gerlach commented Jun 11, 2023

Sidenote: You're missing the required [build-system] section in the bm_2to3/pyproject.toml—might want to add that as appropriate (unless there's a specific reason it's missing?). E.g. if you're using Setuptools:

[build-system]
requires = ["setuptools>=61"]
build-backend = "setuptools.build_meta"

I'm guessing maybe all the other ones might be missing it too, though, so perhaps something to discuss and implement separately.

corona10 and others added 2 commits June 12, 2023 06:56
@corona10 corona10 requested a review from CAM-Gerlach June 12, 2023 01:57
Copy link
Member
@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

LGTM from me as far as a packaging is concerned 👍

@corona10
Copy link
Member Author

Waiting for @gvanrossum 's final review as the 2to3 author and vendoring suggester.

Copy link
Member
@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

I don’t have much expertise here, if CAM is okay you can merge it.

Copy link
Member

Choose a reason for hiding this comment

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

What he said.

@corona10 corona10 merged commit c024b58 into python:main Jun 12, 2023
@corona10 corona10 deleted the gh-294-vendor branch June 12, 2023 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to vendor lib2to3 for CPython 3.13+
4 participants
0