Skip to content

Comments

Meson: raise FeatureNotPresent errors instead of Import errors when dependencies are not available#39412

Draft
tobiasdiez wants to merge 1 commit intosagemath:developfrom
tobiasdiez:meson-optional-pkg
Draft

Meson: raise FeatureNotPresent errors instead of Import errors when dependencies are not available#39412
tobiasdiez wants to merge 1 commit intosagemath:developfrom
tobiasdiez:meson-optional-pkg

Conversation

@tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Jan 30, 2025

Currently, if a dependency of a Cython module is not available, then meson will simply ignore the Cython module. If then a user (or some other parts of sage) tries to import the Cython module, this will fail with a ModuleNotFound error. This is problematic, since there is no indication of the underlying reason (the missing dependency) why the import is not succesful.

This PR proposes to fix this by installing a small dummy python module in place of the original Cython module when the dependencies are not avaiable. This python module then raises a FeatureNotPresent error when one tries to use it.
(In the future, this error may even say what dependency is missing; but this is not yet implemented and needs a few small changes upstream in meson first.)

Instead of raising the FeatureNotPresent error directly upon import, the error is only raised once one actually tries to call the imported method or class. This is done so that the many imports in .all are still "working" and don't raise exceptions. So in short:

# This will never raise an exception
from sage.module.with.missing.dependency import some_fct

# but this will
some_fct("Hello")

This added flexibilty should make it possible to react very dynamically and gracefully when dependencies are missing. This is especially important for Windows, where a bunch of dependencies are just not available (and this will also not change in the near future); see #38872 (comment).

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@tobiasdiez
Copy link
Contributor Author

This is not yet really ready for view but I would appreciate early feedback on the general idea/direction @orlitzky @dimpase @kwankyu @tornaria @antonio-rojas @saraedum

@dimpase
Copy link
Member

dimpase commented Jan 30, 2025

sounds like a good idea

@antonio-rojas
Copy link
Contributor

It is still possible to get ImportErrors if the libraries were present at build time but not at run time, for whatever reason. I'd suggest to keep the ImportError checks alongside the FeatureNotPresent ones to provide a sane fallback in those cases.

@orlitzky
Copy link
Contributor

Will the sage.libs.foo feature tests still work? Right now they do an import and look for ImportError. Or would we need separate checks for modules that appear to work but later throw FeatureNotPresent when you try to use them? (Imports for external cython packages would still fail with an ImportError.)

It's a pretty invasive change, so whether or not it's worth it ultimately depends on what is gained. For the error messages, it's not used consistently, but there is Feature.require() to convert an ImportError into an FeatureNotPresentError. I gather that you want to be able to run sage on Windows and have it not crash, and have all failing tests (due to missing imports) automatically skipped. But will you be able to do anything meaningful when sage starts? And if you can do meaningful work without these imports at startup... couldn't we lazy_import them instead?

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.

4 participants