-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Revert using libCST for dep inference due to performance #11001
Conversation
…10907) # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
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.
lgtm
# We will also fail to parse Python 3.8 syntax if Pants is run with Python 3.6 or 3.7. | ||
# There is no known workaround for this, beyond users changing their `./pants` script to | ||
# always use >= 3.8. |
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.
Is there a way to detect this condition and flag it to a user?
Also I assume this is documented?
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.
Would a package like https://pypi.org/project/ast-compat/ be useful?
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.
Would a package like https://pypi.org/project/ast-compat/ be useful?
Something like that would be good, but that doesn't look production ready nor can I figure out how to use it. Generally, most projects use typed-ast, which works great expect that they decided to not add Py38 support to it. I've heard of libCST and RedBaron too, which are both too slow.
Unfortunately, I think the best we can do is typed-ast, or running with Py38. (If run with Py38, it supports parsing all prior Py3 versions)
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.
Yeah we should probably run Pants CI on 3.8 huh...
…10907) (pantsbuild#11001) Unfortunately, libCST resulted in a major slowdown. Before, ff02ef0: ``` ▶ /usr/bin/time ./pants --no-pantsd typecheck src/python/pants:: ... 10.63 real 11.47 user 6.99 sys ``` After, ea542ac: ``` ▶ /usr/bin/time ./pants --no-pantsd typecheck src/python/pants:: ... 35.40 real 36.13 user 7.08 sys ``` This makes sense: libCST stores a Concrete Syntax Tree, rather than an AST. It's doing more work, like preserving whitespace. Unfortunately, this means that you must again run Pants with Python 3.8 in order to parse Python 3.8-only syntax. Because the original fix results in a 240% slowdown, this is less offensive than the slowdown, even though it's not ideal. [ci skip-rust] [ci skip-build-wheels]
Internal-only changes: * upgrade to cpython crate v0.5.1 (#11052) `PR #11052 <https://github.com/pantsbuild/pants/pull/11052>`_ * Prepare 2.0.0 (#11053) `PR #11053 <https://github.com/pantsbuild/pants/pull/11053>`_ * Revert "Add new EngineAware method metadata() (#11030)" (#11047) `PR #11030 <https://github.com/pantsbuild/pants/pull/11030>`_ `PR #11047 <https://github.com/pantsbuild/pants/pull/11047>`_ * Remove deprecated `python_binary` target in favor of `pex_binary` (#11046) `PR #11046 <https://github.com/pantsbuild/pants/pull/11046>`_ * Prepare 2.0.0rc3 (#11044) `PR #11044 <https://github.com/pantsbuild/pants/pull/11044>`_ * Eagerly validate entry points for `setup_py().with_binaries()` (#11034) `PR #11034 <https://github.com/pantsbuild/pants/pull/11034>`_ `PR #11021 <https://github.com/pantsbuild/pants/pull/11021>`_ * Use Ubuntu Bionic for CI (#11027) `PR #11027 <https://github.com/pantsbuild/pants/pull/11027>`_ * Prepare 2.0.0rc2 (#11017) `PR #11017 <https://github.com/pantsbuild/pants/pull/11017>`_ * Remove RunTrackerLogger (#11018) `PR #11018 <https://github.com/pantsbuild/pants/pull/11018>`_ * Upgrade Pex to 2.1.20 (#11014) `PR #11014 <https://github.com/pantsbuild/pants/pull/11014>`_ * Remove more unused code from RunTracker (#11012) `PR #11012 <https://github.com/pantsbuild/pants/pull/11012>`_ * Add type annotations to AggregatedTimings (#11009) `PR #11009 <https://github.com/pantsbuild/pants/pull/11009>`_ * Increase default `[python-setup].resolver_jobs` to `cpu_count / 2` (#11006) `PR #11006 <https://github.com/pantsbuild/pants/pull/11006>`_ * Include `<PYENV>` in `[python-setup].interpreter_search_paths` default (#10998) `PR #10998 <https://github.com/pantsbuild/pants/pull/10998>`_ * Remove PantsDaemonStats class wrapper (#11003) `PR #11003 <https://github.com/pantsbuild/pants/pull/11003>`_ `PR #files#r508861045 <https://github.com/pantsbuild/pants/pull/11000/files#r508861045>`_ * Revert using libCST for dep inference due to performance (#10907) (#11001) `PR #10907 <https://github.com/pantsbuild/pants/pull/10907>`_ `PR #11001 <https://github.com/pantsbuild/pants/pull/11001>`_ * Run tracker refactor (#11000) `PR #11000 <https://github.com/pantsbuild/pants/pull/11000>`_ * refactor Core::new including command runner setup (#10993) `PR #10993 <https://github.com/pantsbuild/pants/pull/10993>`_ `PR #10960 <https://github.com/pantsbuild/pants/pull/10960>`_ * Allow changing the versioning scheme for `python_distribution` first-party dependencies (#10977) `PR #10977 <https://github.com/pantsbuild/pants/pull/10977>`_ * Remove tokio Handle type from Executor::new (#10980) `PR #10980 <https://github.com/pantsbuild/pants/pull/10980>`_ * Remove deprecated `Address.parse()` and `Address.reference()` (#10981) `PR #10981 <https://github.com/pantsbuild/pants/pull/10981>`_ * Remove project_ methods from the externs module (#10955) `PR #10955 <https://github.com/pantsbuild/pants/pull/10955>`_ * Prepare 2.0.0rc1 (#10972) `PR #10972 <https://github.com/pantsbuild/pants/pull/10972>`_ * Fix interpreter selection when building a PEX to use `[python-setup].interpreter_search_paths` (#10965) `PR #10965 <https://github.com/pantsbuild/pants/pull/10965>`_ * Delete `JsonReporter` (#10964) `PR #10964 <https://github.com/pantsbuild/pants/pull/10964>`_ * Fix log (#10959) `PR #10959 <https://github.com/pantsbuild/pants/pull/10959>`_ * Expose getattr method for Python-related APIs (#10953) `PR #10953 <https://github.com/pantsbuild/pants/pull/10953>`_ * Upgrade tokio package to 0.2.22 (#10949) `PR #10949 <https://github.com/pantsbuild/pants/pull/10949>`_ * Use `package` to build Pants's wheels, rather than `setup-py` (#10947) `PR #10947 <https://github.com/pantsbuild/pants/pull/10947>`_ * Simplify val_to_str and have it and val_to_log_level use PyObject (#10946) `PR #10946 <https://github.com/pantsbuild/pants/pull/10946>`_
Unfortunately, libCST resulted in a major slowdown.
Before, ff02ef0:
After, ea542ac:
This makes sense: libCST stores a Concrete Syntax Tree, rather than an AST. It's doing more work, like preserving whitespace.
Unfortunately, this means that you must again run Pants with Python 3.8 in order to parse Python 3.8-only syntax. Because the original fix results in a 240% slowdown, this is less offensive than the slowdown, even though it's not ideal.
[ci skip-rust]
[ci skip-build-wheels]