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

Remove deprecated Cython IF, DEF #1277

Merged
merged 15 commits into from
Oct 27, 2023

Conversation

ZedThree
Copy link
Contributor

Fixes #1269

The main trick I've used here is having a new C header netcdf-compat.h which does two things:

  1. moves most of the API checking out of setup.py
  2. defines stubs for the missing functions when a feature is missing

This allows us to eliminate most of the DEF statements and compile-time IF conditionals. We can also include everything unconditionally from this compatibility header in netCDF4.pxi. The use of the functions is still guarded at runtime, and I've flipped most of these guards to raise exceptions as soon as possible. The stubs all return NC_EINVAL so if for some reason a guard is missed or bypassed, they'll still raise an error and not silently fail.

I needed to use a different technique for the conditional cimport of mpi4py. For this, I moved the two branches into separate files, [no_]parallel_support_imports.pxi.in. The build system then chooses which one to copy to parallel_support_imports.pxi which we can then unconditionally include in the .pyx file.

This helps remove lots of the deprecated Cython IF/DEF statements
We only need to know if we have any parallel support at compile time,
and not whether it's netcdf4 or pnetcdf flavour
To avoid the last deprecated compile-time `IF`, we instead separate
the two branches of the conditional into different files and choose
which one to include at build time.
We also need to explicitly cast the comm/info arguments to the correct type
@jswhit
Copy link
Collaborator

jswhit commented Oct 2, 2023

Thanks for working on this! Since this is a pretty big change, and the IF/DEF support in cython is not going away anytime soon, I'd like to leave this open for discussion for a while before merging (after the next release perhaps).

@ZedThree
Copy link
Contributor Author

ZedThree commented Oct 3, 2023

Sure, that sounds reasonable! I'm happy to break it into smaller PRs if you prefer

@jswhit
Copy link
Collaborator

jswhit commented Oct 10, 2023

I don't see any reason to split it up, but we'll see what others think.

@jswhit
Copy link
Collaborator

jswhit commented Oct 26, 2023

Now that 1.6.5 is out, this can be merged as soon as it is ready. @ZedThree please bump the version number to 1.7.0 and add a Changelog entry.

* master:
  include fix to pyproject.toml for 3.12
  fix missing part of previous commit
  fix for pkg_resources missing on 3.12
  version 1.6.5 release
  remove 'oversubscribe' option from mpirun
  fix typo
  add python 3.12, remove python 3.7
No longer needed
@ZedThree
Copy link
Contributor Author

I think this is ready now

@jswhit jswhit merged commit 908220b into Unidata:master Oct 27, 2023
29 checks passed
jswhit added a commit that referenced this pull request Apr 17, 2024
ocefpaf pushed a commit to ocefpaf/netcdf4-python that referenced this pull request Apr 26, 2024
Fix bug in set_collective added in commit f64ee26 that cause access always be in collective mode.
Independent access mode is not tested so it passed unnoticed.
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.

IF/DEF conditional compilation deprecated in cython 3.0
2 participants