Skip to content

bpo-40669: Install PEG benchmarking dependencies in a venv #20183

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 2 commits into from
May 18, 2020

Conversation

lysnikolaou
Copy link
Member

@lysnikolaou lysnikolaou commented May 18, 2020

Create a make venv target, that creates a virtual environment
and installs the dependency in that venv. make time and all
the related targets are changed to use the virtual environment
python.

https://bugs.python.org/issue40669

Automerge-Triggered-By: @pablogsal

Create a `make venv` target, that creates a virtual environment
and installs the dependency in that venv. `make time` and all
the related targets are changed to use the virtual environment
python.
@pablogsal
Copy link
Member

Btw, while testing this PR I found that some of the benchmark scripts are broken (for example make time_stdlib_parse):

The venv has been created in the ./venv directory
./venv/bin/python scripts/benchmark.py --parser=cpython --target=xxl parse
Traceback (most recent call last):
  File "/home/pablogsal/github/python/master/Tools/peg_generator/scripts/benchmark.py", line 17, in <module>
    from peg_extension import parse
ImportError: cannot import name 'parse' from 'peg_extension' (/home/pablogsal/github/python/master/Tools/peg_generator/peg_extension/__init__.py)
make: *** [Makefile:87: time_stdlib_parse] Error 1

given that we are focusing on the Python parser now, maybe we could adapt those to use the _peg_parser extension instead of trying to compile the parser.

@lysnikolaou
Copy link
Member Author

Btw, while testing this PR I found that some of the benchmark scripts are broken (for example make time_stdlib_parse):

The venv has been created in the ./venv directory
./venv/bin/python scripts/benchmark.py --parser=cpython --target=xxl parse
Traceback (most recent call last):
  File "/home/pablogsal/github/python/master/Tools/peg_generator/scripts/benchmark.py", line 17, in <module>
    from peg_extension import parse
ImportError: cannot import name 'parse' from 'peg_extension' (/home/pablogsal/github/python/master/Tools/peg_generator/peg_extension/__init__.py)
make: *** [Makefile:87: time_stdlib_parse] Error 1

given that we are focusing on the Python parser now, maybe we could adapt those to use the _peg_parser extension instead of trying to compile the parser.

The problem is that _peg_parser in its current state only parses and generates the external AST. It cannot compile to byte code and it cannot just parse the internal AST. Should we just add build to the dependencies of make time_stdlib_parse?

@pablogsal
Copy link
Member

pablogsal commented May 18, 2020

The problem is that _peg_parser in its current state only parses and generates the external AST.

I see. In any case I suppose that comparing with the old parser will be something that we don't want to do anymore (and the old parser will be removed for the 3.10 series) so I wonder how many of these scripts should be deleted or just ported to use ast.parse or compile directly. For instance, the scripts/test_pypi_packages.py has some logic to compile the parser that is not needed now that is the default one.

Could we do a review of the scripts we want to keep around and delete the rest?

@lysnikolaou
Copy link
Member Author

Could we do a review of the scripts we want to keep around and delete the rest?

Added to my to-do list!

Should we merge this as-is?

@pablogsal
Copy link
Member

Should we merge this as-is?

Yeah, I think is still an improvement 👌

@miss-islington
Copy link
Contributor

@lysnikolaou: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit dc31800 into python:master May 18, 2020
@lysnikolaou lysnikolaou deleted the peg-generator-venv branch May 18, 2020 18:28
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
…20183)

Create a `make venv` target, that creates a virtual environment
and installs the dependency in that venv. `make time` and all
the related targets are changed to use the virtual environment
python.

Automerge-Triggered-By: @pablogsal
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.

6 participants