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

ENH: F2PY refactor #20481

Closed
wants to merge 15 commits into from
Closed

ENH: F2PY refactor #20481

wants to merge 15 commits into from

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Nov 28, 2021

This essentially is the bare minimum set of changes required to use a the new logical structure.

  • Use a new package layout
  • Minimal distutils changes
  • Reduced cruft in __init__.py
  • Switched to absolute imports everywhere
f2py/frontend/crackfortran.py
f2py/frontend/f2py2e.py
f2py/codegen/rules.py
f2py/codegen/cfuncs.py
f2py/codegen/func2subr.py
f2py/utils/diagnose.py
f2py/utils/npdist.py
f2py/utils/pathhelper.py
f2py/stds/f77/common_rules.py
f2py/stds/f90/f90mod_rules.py
f2py/stds/f90/use_rules.py
f2py/stds/pyf/capi_maps.py
f2py/stds/pyf/cb_rules.py
f2py/stds/auxfuncs.py
f2py/stds/symbolic.py
f2py/csrcs/fortranobject.c
f2py/csrcs/fortranobject.h

List of changes

  • Refactor
  • Add a module helper
  • Ensure compatibility
  • Test module helper

Rationale

@HaoZeke
Copy link
Member Author

HaoZeke commented Nov 28, 2021

Note that this PR would break every existing f2py PR, but if it looks good I can go over and rebase all open PRs.

@HaoZeke HaoZeke force-pushed the f2pyRefactor branch 2 times, most recently from becc869 to a6162bb Compare December 2, 2021 16:35
@HaoZeke
Copy link
Member Author

HaoZeke commented Dec 3, 2021

Local reproducer:

python runtests.py -t numpy/tests/test_public_api.py
python runtests.py  -t numpy/typing/tests/test_typing.py::test_code_runs[modules] -m full

@HaoZeke HaoZeke force-pushed the f2pyRefactor branch 2 times, most recently from bb2eab1 to 385864d Compare December 3, 2021 16:11
@HaoZeke
Copy link
Member Author

HaoZeke commented Dec 3, 2021

AFAIK the current build error seems to be a bit of a fluke (CI timeout)

@melissawm
Copy link
Member

There's a few conflicts now, can you check @HaoZeke ?

@HaoZeke HaoZeke force-pushed the f2pyRefactor branch 2 times, most recently from c583a1b to 32a27b6 Compare December 9, 2021 11:54
@HaoZeke
Copy link
Member Author

HaoZeke commented Dec 9, 2021

There's a few conflicts now, can you check @HaoZeke ?

Should be good to go ^_^

@charris
Copy link
Member

charris commented Dec 23, 2021

@HaoZeke Needs another rebase.

@HaoZeke
Copy link
Member Author

HaoZeke commented Dec 24, 2021

I'm not sure this should be merged as is and followed up with:

  1. a better plan for the Public / Private aspects of the API
  2. a shim for ensuring backwards compatibility for existing users

Although, I don't want this PR to get too long...

@HaoZeke
Copy link
Member Author

HaoZeke commented Feb 14, 2022

@pearu and @melissawm, this should be good to go now at last.

@melissawm
Copy link
Member

There's a few tests failures now

@HaoZeke
Copy link
Member Author

HaoZeke commented Feb 16, 2022

This should now pass all the tests. As it stands it might be a bit large, but it can be broken conceptually into two separate parts (was implemented separately too):

  1. The deprecation helper --> Part of this is kanged, and simplified for the most part; the main reason this is needed is that projects often call from numpy.f2py import crackfortran
  2. The actual refactoring --> This involved the renaming of certain symbols to ensure tests pass

As for adding to the undocumented dictionary, not happy about it, but in an ideal world that entire file can be gone in 3 releases...

@HaoZeke HaoZeke force-pushed the f2pyRefactor branch 2 times, most recently from 1d86649 to 08cecf2 Compare February 16, 2022 23:23
@HaoZeke
Copy link
Member Author

HaoZeke commented Feb 21, 2022

Things to test:

  • f90wrap
  • scipy

@HaoZeke
Copy link
Member Author

HaoZeke commented Feb 21, 2022

module deprecation utility

This is a paraphrased version of the Cirq implementation document. The implementation relies heavily on Python's loading mechanisms using importlib.

At a high level:

  • Intercept call to old module (by adding DeprecatedModuleFinder to sys.meta_path)
    • Check that the new module is valid
  • Call DeprecatedModuleLoader which in turn wraps exec_method to ensure both paths resolve to the same ModuleSpec in sys.modules
    • Aliases are added to sys.modules cache recursively

The ModuleSpec specification (PEP 451) ensures that the the module is not actually loaded until needed.

The interface to this is simply:

deprecated_submodule(new_module_name = "numpy.f2py.stds.auxfuncs",
                     old_parent = "numpy.f2py",
                     old_child = "auxfuncs",
                     deadline = "1.25")
  • It will raise after 1.25
  • No spurious files to purge

Additionally, this cleanly handles the case of where modules may be doubly executed otherwise e.g. scikit-learn/scikit-learn#15842:

import numpy as np
from np import linalg # error
sys.modules['np'] = np
from np import linalg # works
from numpy import linalg # executes linalg again

This situation can be (and has been) handled in by using the Loader mechanism.

Alternatives

Stub file generation

This is the approach taken by scikit-learn as demonstrated here (since removed), as noted by @rgommers.

  • Stub files are generated during the installation
  • These import the entire public API
  • It does not seem to extend to being backwards compatible with only loading some part of the old API (e.g. from x import y) as the entire module is executed
  • Unclear in a first read if this supports as (e.g. import y as z)

Biased comparison

IMO, it is cleaner to not generate files which are not needed and can easily be done away with. Additionally, it feels more pythonic to use the standard library's import mechanisms.

Finally, the question of aliases and other double execution of modules are also not cleanly resolved otherwise.

@rgommers
Copy link
Member

IMO, it is cleaner to not generate files which are not needed and can easily be done away with. Additionally, it feels more pythonic to use the standard library's import mechanisms.

This sounds nice in theory, but importlib has been extremely buggy, and this code is too complex. I have a very strong preference for not merging it.

The scikit-learn and SciPy (which does a similar module-level __getattr__ thing, just not with codegen) is much easier to understand.

  1. The deprecation helper --> Part of this is kanged, and simplified for the most part; the main reason this is needed is that projects often call from numpy.f2py import crackfortran

This makes things easier: how about you just commit a single crackfortran.py file with a __getattr__, and be done with it? Nothing else here seems of much use for third-party code and it's not public API, so you're fine just refactoring it without providing shims.

@HaoZeke HaoZeke force-pushed the f2pyRefactor branch 2 times, most recently from 80f7eaa to 83c80ad Compare June 5, 2022 15:18
@HaoZeke
Copy link
Member Author

HaoZeke commented Aug 27, 2023

Closing as discussed in #20480.

@HaoZeke HaoZeke closed this Aug 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Descoped
Development

Successfully merging this pull request may close these issues.

ENH,MAINT: Add a shim for F2PY refactors ENH: Refactor the F2PY module
4 participants