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

CYTHON_LIMITED_API: Fix import utility code #5549

Merged
merged 1 commit into from Jul 20, 2023

Conversation

dalcinl
Copy link
Member

@dalcinl dalcinl commented Jul 20, 2023

If enabling CYTHON_LIMITED_API, relative imports are failing the following way:

$ python3 -c 'from mpi4py import MPI'
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "src/mpi4py/MPI/MPI.pyx", line 12, in init mpi4py.MPI
    bootstrap()
  File "src/mpi4py/MPI/atimport.pxi", line 291, in mpi4py.MPI.bootstrap
    getOptions(&options)
  File "src/mpi4py/MPI/atimport.pxi", line 139, in mpi4py.MPI.getOptions
    from . import rc
KeyError: "'__name__' not in globals"

After looking elsewhere in utility codes, looks like using $moddict_cname with CYTHON_COMPILING_IN_LIMITED_API enabled should work by now after all the changes to support per-module state.

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.

Probably better than the hack we had. Note that we are not running the file tests in the limited API configs, so while I'm definitely interested in what CI has to say, it might not be entirely conclusive.

@scoder scoder added this to the 3.0 milestone Jul 20, 2023
@dalcinl
Copy link
Member Author

dalcinl commented Jul 20, 2023

Note that we are not running the file tests in the limited API configs

Perhaps I should add a specific test for relative imports with CYTHON_COMPILING_IN_LIMITED_API enabled?

@da-woods
Copy link
Contributor

Note that we are not running the file tests in the limited API configs

Perhaps I should add a specific test for relative imports with CYTHON_COMPILING_IN_LIMITED_API enabled?

I was considering doing something similar in the near future for some limited api fixes that are probably hard to test with the present setup. I don't think we want too many of these tests, but I think they're OK as a temporary approach

@dalcinl
Copy link
Member Author

dalcinl commented Jul 20, 2023

I was considering doing something similar in the near future for some limited api fixes that are probably hard to test with the present setup.

Perhaps we should rather add something like an opt-in list of selected tests that we would like to run under the limited C API in addition to the regular run? If we had that, then I would just make sure to add relative import tests that trigger the failure mode I'm fixing in this PR. Given that we already have lots of tests, it is a bit silly to add special tests specific to Cython limited API. Therefore, I believe it is better to not pollute the testsuite and just merge the PR as is.

@da-woods If you are OK with this PR, please merge.

@da-woods da-woods merged commit 0ad32ea into cython:master Jul 20, 2023
73 of 75 checks passed
@scoder
Copy link
Contributor

scoder commented Jul 20, 2023

an opt-in list of selected tests that we would like to run under the limited C API

We usually do it the other way round: provide a file listing "known to be broken" tests for a specific target. I don't see why we should do it differently for the Limited API target.

@dalcinl
Copy link
Member Author

dalcinl commented Jul 20, 2023

I don't see why we should do it differently for the Limited API target.

Sorry, @scoder, but I'm confused. What's the ultimate whish? Run as much tests with limited API as possible (almost effectively doubling the required CI time)? Or just run a few selected tests with limited API when we get a regression report? I understood that @da-woods was somehow proposing the second.

@da-woods
Copy link
Contributor

The ultimate goal is that most of the tests end up running in the limited API. The reason that doesn't happen right now is because it ends up crashing at some point (probably a small reference counting error that's hard to track down).

I think we're a little way off that because there's a lot that doesn't work.

I was proposing running a small number of extra tests for the limited API in the interim, just to test a few things that we have working fully. With the intention of getting rid of those tests when it works properly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants