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

GH-113464: Add a warning when building the JIT #118481

Merged
merged 2 commits into from
May 1, 2024

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented May 1, 2024

PEP 744 makes a bunch of promises once "the JIT builds successfully without displaying warnings to the user". We don't currently do that... so we should.

This PR adds a small banner during JIT builds reminding the user that JIT support for their platform is still experimental, and asking them to report any bugs. This banner is on by default, but can be disabled per-target as they become stable.

This PR also includes a few mostly-cosmetic changes to appease mypy, black, and pylint. They're in separate commits, so they're easier to review.

@brandtbucher brandtbucher added skip news build The build process and cross-build labels May 1, 2024
@brandtbucher brandtbucher self-assigned this May 1, 2024
@bedevere-app bedevere-app bot mentioned this pull request May 1, 2024
@brandtbucher
Copy link
Member Author

==========================================================
JIT support for x86_64-pc-linux-gnu is still experimental!
         Please report any issues you encounter.          
==========================================================

Copy link
Member

@savannahostrowski savannahostrowski left a comment

Choose a reason for hiding this comment

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

LGTM! Though, I do wonder if it's worth adding commit rules or similar to catch formatting issues? Would be nice to just have this tackled automagically?

@brandtbucher
Copy link
Member Author

Though, I do wonder if it's worth adding commit rules or similar to catch formatting issues? Would be nice to just have this tackled automagically?

Agreed.

There's already a mypy job that runs over these files as part of CI. It was easy to add since it already existed for other parts of the codebase. Adding black and pylint would mean actually doing some work and adding new jobs, which I wasn't up for at the time (plus they tend to be more "style" than "correctness", so they feel less important).

@hugovk
Copy link
Member

hugovk commented May 1, 2024

Adding black and pylint would mean actually doing some work and adding new jobs, which I wasn't up for at the time

They can be added in .pre-commit-config.yaml. Make sure to set files: so it only runs on JIT code.

We can add Black to pre-commit (after the Ruff config) like:

  - repo: https://github.com/psf/black-pre-commit-mirror
    rev: 24.4.2
    hooks:
      - id: black
    files: TODO

We can run pylint via the existing Ruff by selecting the PL rules in a local .ruff.toml in the JIT directory, then add that to the pre-commit config. You can compare the Lib/test and Argument Clinic ones which also have their own .ruff.toml.

(cc @AlexWaygood FYI)

(plus they tend to be more "style" than "correctness", so they feel less important)

(Yeah, I usually only run pylint occasionally and don't in CI because it's quite noisy, but if you've resolved them and want to maintain that, I recommend doing it this way.)

@savannahostrowski
Copy link
Member

@brandtbucher If you think that'd be nice, I can take a look at adding a job to do this in .pre-commit-config.yaml for JIT files.

@brandtbucher
Copy link
Member Author

That would be great, yeah. I'm not picky about the set of rules itself, so if the defaults pass then we can just use those.

@brandtbucher brandtbucher enabled auto-merge (squash) May 1, 2024 20:01
@brandtbucher brandtbucher disabled auto-merge May 1, 2024 20:48
@brandtbucher brandtbucher enabled auto-merge (squash) May 1, 2024 20:50
@brandtbucher brandtbucher merged commit 424438b into python:main May 1, 2024
59 checks passed
@gvanrossum
Copy link
Member

This banner is printed even when using --enable-experimental-jit=interpreter. That seems wrong?

@brandtbucher
Copy link
Member Author

Hm. This script should only be part of the Makefile if we're actually building the JIT. It looks like jit_stencils.h is currently generated if the option is anything other than no currently.

So we should update configure.ac to only build the JIT if the option is yes or yes-off.

@gvanrossum
Copy link
Member

Hm. This script should only be part of the Makefile if we're actually building the JIT. It looks like jit_stencils.h is currently generated if the option is anything other than no currently.

So we should update configure.ac to only build the JIT if the option is yes or yes-off.

Yeah, I'm on it.

@brandtbucher
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build skip news topic-JIT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants