Skip to content
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

Install pyright from PyPI #11575

Merged
merged 21 commits into from
Mar 16, 2024
Merged

Install pyright from PyPI #11575

merged 21 commits into from
Mar 16, 2024

Conversation

srittau
Copy link
Collaborator

@srittau srittau commented Mar 11, 2024

No description provided.

@srittau
Copy link
Collaborator Author

srittau commented Mar 11, 2024

Another alternative to #11564 and #11565. Cc @Avasam @AlexWaygood

@srittau
Copy link
Collaborator Author

srittau commented Mar 11, 2024

There's still some problems in CI it seems, but I'd like some feedback whether this is a viable solution before I try to fix this.

@JelleZijlstra
Copy link
Member

I'm wary of doing this since the pip-installable package is an unofficial wrapper and it would be preferable to use the official version that @erictraut releases.

@AlexWaygood
Copy link
Member

I get a pretty long and unhelpful traceback if I pip install pyright and then try running it:

(.venv) [1] % pyright -h                                       ~/dev/typeshed
/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py:26: DeprecationWarning: 'pipes' is deprecated and slated for removal in Python 3.13
  import pipes
/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py:48: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  from pkg_resources import parse_version
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1344, in do_open
    h.request(req.get_method(), req.selector, req.data, headers,
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1331, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1377, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1326, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1085, in _send_output
    self.send(msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1029, in send
    self.connect()
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1472, in connect
    self.sock = self._context.wrap_socket(self.sock,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 455, in wrap_socket
    return self.sslsocket_class._create(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 1042, in _create
    self.do_handshake()
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 1320, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1540, in <module>
    main()
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1111, in main
    args.node = get_last_stable_node_version()
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1044, in get_last_stable_node_version
    return _get_versions_json()[0]['version'].lstrip('v')
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1020, in _get_versions_json
    response = urlopen('%s/index.json' % src_base_url)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 640, in urlopen
    return urllib2.urlopen(req)
           ^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 215, in urlopen
    return opener.open(url, data, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 515, in open
    response = self._open(req, data)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 532, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 492, in _call_chain
    result = func(*args)
             ^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1392, in https_open
    return self.do_open(http.client.HTTPSConnection, req,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1347, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)>
Traceback (most recent call last):
  File "/Users/alexw/dev/typeshed/.venv/bin/pyright", line 8, in <module>
    sys.exit(entrypoint())
             ^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 34, in entrypoint
    sys.exit(main(sys.argv[1:]))
             ^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 19, in main
    return run(*args, **kwargs).returncode
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 25, in run
    pkg_dir = install_pyright(args, quiet=None)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/_utils.py", line 64, in install_pyright
    node.run(
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 105, in run
    binary = _ensure_available(target)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 38, in _ensure_available
    return Binary(path=_ensure_node_env(target), strategy=Strategy.NODEENV)
                       ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 65, in _ensure_node_env
    _install_node_env()
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 98, in _install_node_env
    subprocess.run(args, check=True)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/Users/alexw/dev/typeshed/.venv/bin/python', '-m', 'nodeenv', '/Users/alexw/.cache/pyright-python/nodeenv']' returned non-zero exit status 1.

I think you have to have node/npm/both installed for it to work properly? But that won't be obvious to contributors

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

There's some changes missing in scripts/runtests.py, you should remove the npx-related check since we can now expect the pyright executable to always be available from the requirements install.

Overall I think this is an improvement in terms of complexity that could work standalone from #11564 and #11565 . (assuming all CI issues get resolved, no comment on exactly how we should obtain the pyright version yet). But there can be concern about using a wrapper package over the official one.

And it removes a concern about dependabot, so this should be kept in mind when comparing dependabot vs renovate.
If we wanna go renovate either way, the value of this PR is then slightly reduced.

with:
file: "pyproject.toml"
field: "tool.typeshed.pyright_version"
run: echo PYRIGHT_VERSION=$(pyright --version | cat | head -n 1 | cut -d " " -f 2) >> $GITHUB_ENV
Copy link
Collaborator

@Avasam Avasam Mar 11, 2024

Choose a reason for hiding this comment

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

The CI issue is simple, the version obtention isn't working so it's fetching the latest version

image

Sidenote, I think I preferred using the step output $GITHUB_OUTPUT, which also keeps the rest of the file the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this evaluates to the empty string, which I'm pretty sure I can't distinguish in the action (as empty string is the default value I have to apply to have a parameter of type string for the action).

Copy link
Contributor

@jakebailey jakebailey Mar 11, 2024

Choose a reason for hiding this comment

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

Maybe to prevent this issue, I should just default it to the string "latest" instead, and hope that nobody breaks downstream... I can at least probably do that and warn on empty string in the short term while keeping the same behavior.

Copy link
Collaborator

@Avasam Avasam Mar 11, 2024

Choose a reason for hiding this comment

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

Even with the string "latest", the CI would still fail. Typeshed isn't ready for pyright>=1.1.351 . That's what the pyright-related CI failures are.

(the stubtest one is a timeout)

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is specifically that {{ env.PYRIGHT_VERSION }} is empty, so that's taken as the default, which is latest, but I think anyone that explicitly passes an empty string into the action probably didn't mean to do that. So, I was just thinking aloud whether or not I could detect this and warn. No action required here!

tests/pyright_test.py Outdated Show resolved Hide resolved
tests/pyright_test.py Show resolved Hide resolved
@Avasam
Copy link
Collaborator

Avasam commented Mar 11, 2024

I think you have to have node/npm/both installed for it to work properly? But that won't be obvious to contributors

It's supposed to install node for you using nodeenv if it detects you don't already have it already (the whole point of the wrapper being so that python devs don't have to deal with the javascript runtime and packaging ecosystem themselves). But looks like the install is failing for you due to some certificate stuff.
An interesting additional failure point.

@AlexWaygood
Copy link
Member

I'm now on MacOS but I remember having similar issues with the PyPI Python wrapper when I was on Windows. It's never worked for me out of the box (though I've never done a deep investigation as to why not).

@srittau
Copy link
Collaborator Author

srittau commented Mar 11, 2024

Yeah, all those issues would need to be worked out if we decide this is a good approach. In general I like the idea of installing pyright from PyPI, because of the consistency with other type checkers and because we don't need to worry much about using nodejs the right way, but I share Jelle's concern about this being an inofficial package and the other issues mentioned here.

@erictraut
Copy link
Contributor

Pyright's pypi wrapper is community-maintained (specifically, by Robert Craigie). He does a great job, and I support the work he does. I think it's safe to say that a majority of pyright users install pyright using this mechanism, so the risk that the wrapper would suddenly go away or stop working is close to zero. For that reason, I'm not concerned if typeshed wants to make use of this mechanism.

FWIW, we use the PyPi wrapper to install pyright in the type specification conformance test suite.

@srittau
Copy link
Collaborator Author

srittau commented Mar 11, 2024

That's great to hear, Eric! I think that alleviates my main concern about this solution. I'll see whether I can work on this tomorrow. @Avasam has given good pointers. (But @everyone: Feel free to commit to this branch, I claim no ownership.)

@srittau
Copy link
Collaborator Author

srittau commented Mar 12, 2024

FWIW, ./tests/pyright_test.py fails for me under Linux, both in this, but also on the main branch.

@srittau srittau marked this pull request as ready for review March 12, 2024 12:56
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
tests/utils.py Outdated Show resolved Hide resolved
@srittau srittau added the project: infrastructure typeshed build, test, documentation, or distribution related label Mar 14, 2024
@srittau
Copy link
Collaborator Author

srittau commented Mar 14, 2024

Another option is to keep installing pyright using npx in tests/pyright_test.py, but get the version from requirements-tests.txt, at least for now. But I think using the pip installed version is a bit more convenient.

@Avasam
Copy link
Collaborator

Avasam commented Mar 14, 2024

Another option is to keep installing pyright using npx in tests/pyright_test.py, but get the version from requirements-tests.txt, at least for now. But I think using the pip installed version is a bit more convenient.

In theory, since pyright-python tries installing node if missing, it also enables running npx (a node CLI tool). So whilst it doesn't simplify as much code as I originally thought, it can still make sense for pyright-python to be a requirement the same way uv is.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

pyright_test.py fails for me (with the PyPI pyright package installed) both on main and with this PR. But on main I get this:

(typeshed) % py tests/pyright_test.py                                                                                                 ~/dev/typeshed
error finding npx; is Node.js installed?

Whereas with this PR branch I get this:

(typeshed) % py tests/pyright_test.py                                                                                                 ~/dev/typeshed
Running: pyright --version
/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py:26: DeprecationWarning: 'pipes' is deprecated and slated for removal in Python 3.13
  import pipes
/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py:48: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html
  from pkg_resources import parse_version
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1344, in do_open
    h.request(req.get_method(), req.selector, req.data, headers,
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1331, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1377, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1326, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1085, in _send_output
    self.send(msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1029, in send
    self.connect()
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/http/client.py", line 1472, in connect
    self.sock = self._context.wrap_socket(self.sock,
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 455, in wrap_socket
    return self.sslsocket_class._create(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 1042, in _create
    self.do_handshake()
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/ssl.py", line 1320, in do_handshake
    self._sslobj.do_handshake()
ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1540, in <module>
    main()
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1111, in main
    args.node = get_last_stable_node_version()
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1044, in get_last_stable_node_version
    return _get_versions_json()[0]['version'].lstrip('v')
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 1020, in _get_versions_json
    response = urlopen('%s/index.json' % src_base_url)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/nodeenv.py", line 640, in urlopen
    return urllib2.urlopen(req)
           ^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 215, in urlopen
    return opener.open(url, data, timeout)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 515, in open
    response = self._open(req, data)
               ^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 532, in _open
    result = self._call_chain(self.handle_open, protocol, protocol +
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 492, in _call_chain
    result = func(*args)
             ^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1392, in https_open
    return self.do_open(http.client.HTTPSConnection, req,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/urllib/request.py", line 1347, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: unable to get local issuer certificate (_ssl.c:1000)>
Traceback (most recent call last):
  File "/Users/alexw/dev/typeshed/.venv/bin/pyright", line 8, in <module>
    sys.exit(entrypoint())
             ^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 34, in entrypoint
    sys.exit(main(sys.argv[1:]))
             ^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 19, in main
    return run(*args, **kwargs).returncode
           ^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/cli.py", line 25, in run
    pkg_dir = install_pyright(args, quiet=None)
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/_utils.py", line 64, in install_pyright
    node.run(
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 105, in run
    binary = _ensure_available(target)
             ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 38, in _ensure_available
    return Binary(path=_ensure_node_env(target), strategy=Strategy.NODEENV)
                       ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 65, in _ensure_node_env
    _install_node_env()
  File "/Users/alexw/dev/typeshed/.venv/lib/python3.12/site-packages/pyright/node.py", line 98, in _install_node_env
    subprocess.run(args, check=True)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/Users/alexw/dev/typeshed/.venv/bin/python', '-m', 'nodeenv', '/Users/alexw/.cache/pyright-python/nodeenv']' returned non-zero exit status 1.
Traceback (most recent call last):
  File "/Users/alexw/dev/typeshed/tests/pyright_test.py", line 35, in <module>
    main()
  File "/Users/alexw/dev/typeshed/tests/pyright_test.py", line 25, in main
    subprocess.run(["pyright", "--version"], check=True)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 571, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['pyright', '--version']' returned non-zero exit status 1.

I don't mind the test failing for me locally, since it also fails on main. But can we make the traceback a bit better there? I feel like a traceback that long would be a horrible experience for contributors trying to run our tests.

@srittau
Copy link
Collaborator Author

srittau commented Mar 15, 2024

These seem to fail for very different reasons. While the main version fails an explicit check for npx being installed, the latter fails for an unexpected reason: Some SSL certificate error. I'm not sure whether it's a good idea to suppress that traceback. In fact, the traceback is quite useful to debug the reason for the failure.

@AlexWaygood
Copy link
Member

That's all true (I agree that the traceback is useful and we shouldn't simply suppress it), but I don't think it changes the fact that it feels like a degradation in user experience for a contributor trying to run the test

@srittau
Copy link
Collaborator Author

srittau commented Mar 15, 2024

The question is: Why are you getting an SSL certificate error? This seems like a networking problem that you also could get while manually installing npx.

@Avasam
Copy link
Collaborator

Avasam commented Mar 15, 2024

The question is: Why are you getting an SSL certificate error? This seems like a networking problem that you also could get while manually installing npx.

Installing node/npm* (sorry, just being pedantic)

Where previously we expected users to install node/npm by themselves using their favored installation method, this now has an expectation that everything is installed for you.

Where previously a dev could omit to install node/npm for various reasons and get a simple "skipped" warning. This now spams a dev for whom the automated install doesn't work.

Just like the previous errors, we could catch and simplify the error. Turning it into a warning (the test didn't fail, you were just unable to run it). But having an option to show the full error (verbose flag?) might be appropriate. So the dev can figure out their issue themselves.
Or we figure out why @AlexWaygood has SSL issues, so we can provide guidance with the warning message.
Speaking of which:

@srittau
Copy link
Collaborator Author

srittau commented Mar 15, 2024

Where previously a dev could omit to install node/npm for various reasons and get a simple "skipped" warning. This now spams a dev for whom the automated install doesn't work.

Please note that this is the pyright_test script. When calling it, I'd expect an error, not a warning, when executing pyright fails. Which is what the previous script did as well. If the SSL error is (expected to be) a common occurence, and not a bug on our side or a temporary failure, I agree that we should have a better error message with hints on how to fix it.

@jakebailey
Copy link
Contributor

Honestly, I am surprised that the pyright PyPI package is running npx at all; pyright is dependency free and can just be pulled as a tarball from the npm registry (e.g. in python) and run with node directly. That's all the pyright-action does.

@Avasam
Copy link
Collaborator

Avasam commented Mar 15, 2024

Please note that this is the pyright_test script. When calling it, I'd expect an error, not a warning, when executing pyright fails. [...]

My bad, I got my wires crossed and I meant a warning when run from scripts/runtests.py, and yeah, if you run the pyright test directly and it fails, kinda makes sense to see the entire error.

@srittau
Copy link
Collaborator Author

srittau commented Mar 15, 2024

@jakebailey As far as I can see, it's not using npx: https://github.com/RobertCraigie/pyright-python/blob/main/pyright/_utils.py#L64-L72

But we are currently using npx in our pyright_test script.

@jakebailey
Copy link
Contributor

Ah, gotcha. I saw some references to npx over there, but it looks like leftover code. It also could skip npm, but I can understand wanting to leverage npm to do version resolution (though doing it just with the registry is not super hard and IMO would be worth it to avoid the dep). But, obviously this PR isn't the place to report that 😄

@srittau
Copy link
Collaborator Author

srittau commented Mar 15, 2024

But anyway, since there are obviously problems with running the pyright version installed with pip on some platforms, I'll partly revert the changes in pyright_test so that pyright is run using npx again. We can investigate the problems in a followup PR.

tests/pyright_test.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

LGTM now. Thanks!

@DetachHead
Copy link
Contributor

you could use basedpyright which solves these issues by bundling the npm package in the wheel

@srittau srittau merged commit a899720 into python:main Mar 16, 2024
88 checks passed
@srittau srittau deleted the pyright-pypi branch March 16, 2024 13:14
@jakebailey
Copy link
Contributor

I know you just (ish) merged this, but one thing I'm considering adding to pyright-action is the ability to specify something like version: PATH which instructs the action to instead use pyright from $PATH.

It looks like you all already install requirements-tests.txt, so this would let the action just grab pyright from there and eliminate any special parsing or dependency management. Is that something you're interested in?

(This should work for both tests.yml and tests_meta.yml, though one uses a venv and the other uses a global install, it seems...)

@srittau
Copy link
Collaborator Author

srittau commented Apr 9, 2024

This sounds quite useful as it would remove the awkward requirements parsing step from the workflows.

@jakebailey
Copy link
Contributor

jakebailey/pyright-action#107 is the PR; for this repo it'd just mean deleting any of the version stuff and adding version: PATH and that's it; the requirements-tests.txt becomes the source of truth. Let me know if anything feels off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: infrastructure typeshed build, test, documentation, or distribution related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set up automation for updating pytype and pyright versions
7 participants