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

stubgen: Fix the detection of modules without spec. #940

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ingomueller-net
Copy link

@ingomueller-net ingomueller-net commented Feb 18, 2025

This PR is stacked on top and, therefor, contains the commits of #939.

This PR changes the logic of is_valid_module used in simplify_types. That function uses importlib.util.find_spec and previously tested the return value for is None. However, (1) if the spec is actually None, then the function doesn't return None but raises ValueError, so that test never succeeded and (2) if a module with a None spec was found, we have still found a module, so is_valid_module should actually return True instead of False.

This PR fixes the logic that tries to find which dot (`.`) separates the
module from the class in a nested type name in `simplify_types`. The
current logic flipped the components, creating invalid type names.

Signed-off-by: Ingo Müller <[email protected]>
This PR changes the logic of `is_valid_module` used in `simplify_types`.
That function uses `importlib.util.find_spec` and previously tested the
return value for `is None`. However, (1) if the spec is actually `None`,
then the function doesn't return `None` but raises `ValueError`, so that
test never succeeded and (2) if a module with a `None` spec was found,
we have still found a module, so `is_valid_module` should actually
return `True` instead of `False`.

Signed-off-by: Ingo Müller <[email protected]>
@ingomueller-net
Copy link
Author

OK, I think I need some help now, or at least input from a discussion. Turns out that the importing of nested modules was broken in two different ways that hid each other. #939 fixes how nested modules are broken up into module and class; this PR fixes the detection of whether a module exists or not. For a module to be valid according to importlib, clearly (right?), it has to be installed. I have added some logic for installing tensorflow and jax, which the tests use types from, and that improves some of the CI targets from the matrix. I guess that those packages are not available for the remaining targets or have to be installed differently. It'd be great if someone with more experience could look into that. Plus, I guess, it'd be great to agree that that's the right thing to do before investing that time.

@wjakob
Copy link
Owner

wjakob commented Feb 18, 2025

I haven't installed any of the big tensor frameworks on the CI since that makes things very slow (plus, there aren't any GPUs there to test non-CPU device paths). That means that stubgen may need to make stubs involving types of a package that hasn't been installed.

@ingomueller-net
Copy link
Author

I understand. This has another advantage: if detecting whether a string represents a valid module depends on the installed packages, then stubgen may have quite different results depending on the system. Plus, even with the current state of the PR, stubgen simply used some invalid module name if the module in question didn't exist (namely the most top-level component of the nested module names).

What is the solution, then? Do it only string-based? For x.y.z, x.y is the the module and we import that? Any inspection would otherwise requirement the modules to exist...

@ingomueller-net
Copy link
Author

What is the solution, then? Do it only string-based? For x.y.z, x.y is the the module and we import that? Any inspection would otherwise requirement the modules to exist...

I don't think that that works. I quickly implemented this; now this case isn't handled correctly anymore: def makeNestedClass() -> py_stub_test.AClass.NestedClass: ... (It tries to import test.AClass, which isn't a module.)

I don't see how this can be handled without inspection of the actual module and I don't think that that, in turn, can be done without installing the packages. One option to handle that latter issue is to skip that test in CI (or in all but one target).

This commit extracts the functions involving JAX and TensorFlow types
from the module in `test_ndarray.cpp` into their own respective modules
and sets up the boilerplate code to turn them into independent tests.
This allows to put the corresponding stub generation tests behind a
flag and deactivate them by default.

Signed-off-by: Ingo Müller <[email protected]>
@ingomueller-net
Copy link
Author

@wjakob, check out the latest version. I moved the JAX- and TensorFlow-related tests into dedicated files and put those behind a flag that isn't enabled in CI. WDYT?

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.

None yet

2 participants