use libCST to parse imports from python 3 code #10907

cosmicexplorer · 2020-10-05T06:57:42Z

Problem

Solution

Add libcst>=0.3.12,<0.4 to requirements.txt.
Remove the usage of the normal python 3 ast library from import_parser.py, and implement a _CSTVisitor to visit import nodes.
When using python 3, instead use the _CSTVisitor.
- We use python 3.8 to parse all python 3 code, which is 100% correct for the purpose of parsing imports.

Result

We should no longer be dependent on the version of the current python executable to parse python code for dependencies!

# 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]

Eric-Arellano

Thanks for working on this!

3rdparty/python/constraints.txt

src/python/pants/backend/python/dependency_inference/rules.py

3rdparty/python/constraints.txt

Eric-Arellano

You can remove this test skip! That's really exciting. This is a great improvement.

pants/src/python/pants/backend/python/dependency_inference/import_parser_test.py

Lines 149 to 153 in 923f461

    
           @pytest.mark.skipif( 
        
               sys.version_info[:2] < (3, 8), 
        
               reason="Cannot parse Python 3.8 unless Pants is run with Python 3.8.", 
        
           ) 
        
           def test_works_with_python38() -> None:

# 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]

coveralls · 2020-10-06T02:35:42Z

Coverage remained the same at 0.0% when pulling d7de279 on cosmicexplorer:libcst-for-range-of-python-versions into ee50791 on pantsbuild:master.

3rdparty/python/requirements.txt

src/python/pants/backend/python/dependency_inference/import_parser.py

Eric-Arellano · 2020-10-06T02:49:11Z

src/python/pants/backend/python/dependency_inference/import_parser.py

+        if isinstance(s, bytes):
+            return


This is because SimpleString includes bytes, I presume?

Yes, although it's not explicitly mentioned in the documentation: https://libcst.readthedocs.io/en/latest/nodes.html?highlight=import#libcst.SimpleString.

With pex:

>>> cst.parse_expression('b"asdf"').evaluated_value b'asdf' >>> type(cst.parse_expression('b"asdf"')) <class 'libcst._nodes.expression.SimpleString'> >>> cst.parse_expression('"asdf"').evaluated_value 'asdf' >>> type(cst.parse_expression('"asdf"')) <class 'libcst._nodes.expression.SimpleString'>

Eric-Arellano · 2020-10-06T03:40:00Z

Also, what do you think about logging at the debug level if the AST failed to parse? I'm thinking about new users trying out Python 3.9 being confused why dep inference isn't working if they are using Py39-only syntax. It might be helpful for us if we could ask them to run ./pants -ldebug so that we can identify it's an AST parsing issue vs. something else, like >1 owner for the file.

If you like that, it would be excellent to use the caplog Pytest fixture to ensure we are logging:

pants/src/python/pants/backend/python/typecheck/mypy/rules_integration_test.py

Lines 735 to 744 in 3b6fbd7

    
           def test_warn_if_python_version_configured(caplog) -> None: 
        
               def assert_is_configured(*, has_config: bool, args: List[str], warning: str) -> None: 
        
                   config = FileContent("mypy.ini", b"[mypy]\npython_version = 3.6") if has_config else None 
        
                   is_configured = check_and_warn_if_python_version_configured(config=config, args=tuple(args)) 
        
                   assert is_configured 
        
                   assert len(caplog.records) == 1 
        
                   assert warning in caplog.text 
        
                   caplog.clear() 
        
               assert_is_configured(has_config=True, args=[], warning="You set `python_version` in mypy.ini")

(I think DEBUG rather than WARN because we don't want to be too noisy. Their code will already eagerly fail if they have a legit syntax error when running run, test, etc. A general principle of dep inference is that it should never cause your build to break; it's only an enhancement.)

# 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]

cosmicexplorer · 2020-10-07T20:13:01Z

Also, what do you think about logging at the debug level if the AST failed to parse?

Done! Thanks so much for suggesting this!

# 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]

Eric-Arellano

Excellent. Thank you so much for iterating on this! I love the debug messages and appreciate the tests for it.

…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]

…1001) 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]

…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]

cosmicexplorer added 2 commits October 4, 2020 22:31

add libCST

4ad57ef

# 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]

use libCST to parse python 3

fd082cb

# 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]

cosmicexplorer force-pushed the libcst-for-range-of-python-versions branch from 284dace to fd082cb Compare October 5, 2020 07:05

Eric-Arellano reviewed Oct 5, 2020

View reviewed changes

3rdparty/python/constraints.txt Outdated Show resolved Hide resolved

src/python/pants/backend/python/dependency_inference/rules.py Outdated Show resolved Hide resolved

Eric-Arellano reviewed Oct 5, 2020

View reviewed changes

3rdparty/python/constraints.txt Show resolved Hide resolved

Eric-Arellano reviewed Oct 5, 2020

View reviewed changes

update lockfile!

e08e438

# 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]

cosmicexplorer force-pushed the libcst-for-range-of-python-versions branch from 2e4e2fe to e08e438 Compare October 6, 2020 01:22

use python 3.8 to parse all python 3 code!

507a3b4

# 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]

cosmicexplorer force-pushed the libcst-for-range-of-python-versions branch from 80ab6d9 to 46459fd Compare October 6, 2020 01:55

remove test skip (!!!!!!!!)

e58a4b1

# 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]

cosmicexplorer force-pushed the libcst-for-range-of-python-versions branch from 46459fd to e58a4b1 Compare October 6, 2020 01:56

Eric-Arellano reviewed Oct 6, 2020

View reviewed changes

cosmicexplorer mentioned this pull request Oct 7, 2020

determine the appropriate interpreter to parse python imports #10921

Closed

cosmicexplorer added the release:features label Oct 7, 2020

cosmicexplorer mentioned this pull request Oct 7, 2020

Support parsing python 3.9/3.10 syntax for dependency inference #10922

Closed

cosmicexplorer added 3 commits October 7, 2020 11:58

parse python 3 code first! and add 2 TODOs

7ed7fa4

# 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]

make libCST requirement float a bit!

f0a9a99

# 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]

make typechecking work

7dab459

# 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]

test debug log!

d7de279

# 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]

cosmicexplorer force-pushed the libcst-for-range-of-python-versions branch from d218329 to d7de279 Compare October 7, 2020 20:13

Eric-Arellano approved these changes Oct 7, 2020

View reviewed changes

cosmicexplorer merged commit ea542ac into pantsbuild:master Oct 8, 2020

gshuflin mentioned this pull request Oct 30, 2020

Prepare 2.1.0.dev0 #11089

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use libCST to parse imports from python 3 code #10907

use libCST to parse imports from python 3 code #10907

cosmicexplorer commented Oct 5, 2020 •

edited

Loading

Eric-Arellano left a comment

Eric-Arellano left a comment

coveralls commented Oct 6, 2020 •

edited

Loading

Eric-Arellano Oct 6, 2020

cosmicexplorer Oct 7, 2020 •

edited

Loading

Eric-Arellano commented Oct 6, 2020 •

edited

Loading

cosmicexplorer commented Oct 7, 2020

Eric-Arellano left a comment

	@pytest.mark.skipif(
	sys.version_info[:2] < (3, 8),
	reason="Cannot parse Python 3.8 unless Pants is run with Python 3.8.",
	)
	def test_works_with_python38() -> None:

use libCST to parse imports from python 3 code #10907

use libCST to parse imports from python 3 code #10907

Conversation

cosmicexplorer commented Oct 5, 2020 • edited Loading

Problem

Solution

Result

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

coveralls commented Oct 6, 2020 • edited Loading

Eric-Arellano Oct 6, 2020

Choose a reason for hiding this comment

cosmicexplorer Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

Eric-Arellano commented Oct 6, 2020 • edited Loading

cosmicexplorer commented Oct 7, 2020

Eric-Arellano left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Oct 5, 2020 •

edited

Loading

coveralls commented Oct 6, 2020 •

edited

Loading

cosmicexplorer Oct 7, 2020 •

edited

Loading

Eric-Arellano commented Oct 6, 2020 •

edited

Loading