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

Generate cache for builtins #6043

Merged
merged 9 commits into from Mar 8, 2024

Conversation

alexandrasp
Copy link
Contributor

@alexandrasp alexandrasp commented Feb 29, 2024

  • Create a set with all attributes available in the most recent python (3.12.2) builtins.

  • Use this cache in Symtab.py in declare_builtin functions.

Fix to #5591

Copy link
Contributor

@da-woods da-woods left a comment

Choose a reason for hiding this comment

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

In terms of tests:

I don't think we have any tests for the previous version of this feature. It's probably the sort of thing that needs to be tested by the content of the C code, which is always a little fiddly.

The way to do that is test_assert_c_code_has or test_fail_if_c_code_has.

Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
@scoder
Copy link
Contributor

scoder commented Feb 29, 2024

Regarding tests, I propose the following:

Add a constant KNOWN_PYTHON_BUILTINS_VERSION next to the set that holds the exact sys.version_info tuple value of the CPython version that you took the list from.

Add a test that compares the version to sys.version_info and skips the test for older Python releases. For that version and newer, it compares the set of builtins to the system list and fails if they differ.

That way, we notice changes in the list of builtins through test failures and can adapt in time.

EDIT: when skipping the test (on older versions), it would still be nice to print any differences in the skip message, so that it's easy to look through build logs for the list of supported/expected builtins of a specific Python version.

- Create a set with all attributes available in
the most recent python (3.12.2) builtins.

- Use this cache in Symtab.py in declare_builtin functions.

Signed-off-by: Alexandra Pereira <alesilva241@gmail.com>
@alexandrasp alexandrasp force-pushed the generate-cache-builtins branch 3 times, most recently from 2468006 to 85b0be0 Compare March 5, 2024 05:19
@alexandrasp
Copy link
Contributor Author

Regarding tests, I propose the following:

Add a constant KNOWN_PYTHON_BUILTINS_VERSION next to the set that holds the exact sys.version_info tuple value of the CPython version that you took the list from.

done!

Add a test that compares the version to sys.version_info and skips the test for older Python releases. For that version and newer, it compares the set of builtins to the system list and fails if they differ.

done! But, I added the test in TestBuiltin.py.

That way, we notice changes in the list of builtins through test failures and can adapt in time.

EDIT: when skipping the test (on older versions), it would still be nice to print any differences in the skip message, so that it's easy to look through build logs for the list of supported/expected builtins of a specific Python version.

I got the difference between the builtin available in the runtime env and the fixed version and printed the list of possible missing builtins if there are any.

- add a constant with the version of CPython that we got the
builtins attributes available.

- add a test that will skip in case of using an older CPython
version in runtime, or pass in case of the same, or newer version with
same attributes.

Signed-off-by: Alexandra Pereira <alesilva241@gmail.com>
@alexandrasp
Copy link
Contributor Author

After checking the errors reported by the CI, I am wondering if the environment used to run the tests could have any additional features, or even if I should use anything different than dir() to obtain __builtins__ attributes.

For example:

ubuntu-20.04, 3.13-dev, c,cpp, --limited-api, --no-file, true, -limited_api

ubuntu-20.04, 3.12, c,cpp, --limited-api, --no-file, true, -limited_api

windows-2019, cpp, 3.12

Skipping seems to be fine in the case of old versions:

windows-2019, c, 3.7

ubuntu-20.04, c, 3.9

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

I just noticed that we didn't keep uncachable_builtins up to date, which holds the list of builtins that change across Python releases and thus cannot be cached globally. I'll update it.

Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

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

Looks like the list (and the test) includes module attributes that are not builtins.

Cython/Compiler/Symtab.py Outdated Show resolved Hide resolved
Cython/Compiler/Tests/TestBuiltin.py Outdated Show resolved Hide resolved
@scoder scoder merged commit da405c6 into cython:master Mar 8, 2024
64 checks passed
scoder added a commit that referenced this pull request Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated cached builtins from a fixed list rather than looking up in the interpreter
3 participants