Fix cross-module scoped enum lookup in multi-module builds #240
Conversation
In multi-module builds (e.g., pyOpenMS with _pyopenms_1.pyx through _pyopenms_8.pyx), scoped enums (enum class) wrapped as Python IntEnum classes could fail when used across modules. Problem: - The previous globals().get() approach only searched the current module - When MSSpectrum (module 4) called getType() returning SpectrumType (defined in module 3), the getter returned a plain int - The setter then failed with "AttributeError: 'int' object has no attribute 'value'" because it expected the IntEnum wrapper Solution: - Add a per-module registry (_scoped_enum_registry) that stores enums defined in that module - Add a lookup function (_get_scoped_enum_class) that: 1. First checks the local registry (fast path for same-module enums) 2. Then searches all loaded pyopenms modules via sys.modules 3. Caches results for subsequent lookups - The helper code is added to top_level_pyx_code so it only appears in .pyx files, not .pxd files (which cannot contain def statements) This fixes issue #239. Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
Warning Rate limit exceeded@timosachsenberg has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a module-local scoped-enum registry and a runtime lookup function (_get_scoped_enum_class), emits helper code from CodeGenerator, updates generated enum conversion to use the new lookup, and adds tests and Cython declarations to validate cross-module enum getter/setter round-trips. Changes
Sequence Diagram(s)sequenceDiagram
participant PyUser as Python caller
participant PyModule as Generated module code (getter/setter)
participant ScopedRegistry as _get_scoped_enum_class / module registry
participant CppObj as C++ instance via Cython
PyUser->>PyModule: call getStatus()
PyModule->>CppObj: call native getter -> int
PyModule->>ScopedRegistry: lookup Python enum wrapper for type
ScopedRegistry-->>PyModule: return EnumClass (or identity)
PyModule-->>PyUser: return EnumClass(int)
PyUser->>PyModule: call setStatus(EnumClass instance)
PyModule->>ScopedRegistry: verify/lookup expected EnumClass
PyModule->>CppObj: convert EnumClass to int and call native setter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
autowrap/CodeGenerator.py (1)
2123-2176: Consider making the package name configurable.The cross-module lookup hardcodes
'pyopenms'at line 2165, which limits this feature to only the pyOpenMS project. For a general-purpose solution that works with other multi-module autowrap projects, consider making the package name configurable or auto-detecting it from the module structure.Possible approaches
Option 1: Infer from current module name
| # Slow path: search all loaded pyopenms modules +| current_module = __name__.split('.')[0] # Get package name from current module | for mod_name, mod in list(_sys.modules.items()): -| if mod is not None and (mod_name.startswith('pyopenms.') or mod_name == 'pyopenms'): +| if mod is not None and (mod_name.startswith(current_module + '.') or mod_name == current_module):Option 2: Search all modules (slower but more general)
| # Slow path: search all loaded modules | for mod_name, mod in list(_sys.modules.items()): -| if mod is not None and (mod_name.startswith('pyopenms.') or mod_name == 'pyopenms'): +| if mod is not None: | enum_cls = getattr(mod, name, None) | if enum_cls is not None:Option 3: Add configuration parameter
Pass the package name as a parameter to the code generator and use it in the template.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
autowrap/CodeGenerator.pyautowrap/ConversionProvider.py
🧰 Additional context used
🧬 Code graph analysis (1)
autowrap/CodeGenerator.py (1)
autowrap/Code.py (2)
add(56-75)Code(41-90)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (==3.2.0, 3.13)
- GitHub Check: test (==3.2.0, 3.11)
- GitHub Check: test (==3.1.0, 3.11)
- GitHub Check: test (==3.2.0, 3.12)
- GitHub Check: test (==3.1.0, 3.10)
- GitHub Check: test (==3.2.0, 3.10)
- GitHub Check: test (==3.1.0, 3.13)
- GitHub Check: test (==3.1.0, 3.12)
🔇 Additional comments (6)
autowrap/CodeGenerator.py (4)
282-282: LGTM! Proper integration of scoped enum helper generation.The call is well-placed in the code generation sequence, after foreign imports and before includes.
525-528: LGTM! Scoped enum registration is correctly placed.Registration occurs immediately after the scoped enum class definition, enabling the fast-path lookup for same-module enum access.
2088-2122: Well-documented no-op approach to avoid circular imports.The extensive documentation clearly explains why module-level imports are avoided and how the runtime lookup solution works instead. This is a good design decision for multi-module builds.
2209-2209: LGTM! Necessary import for cross-module enum lookup.The underscore prefix
_sysis good practice to avoid potential naming conflicts with user code.autowrap/ConversionProvider.py (2)
405-431: LGTM! Proper integration with cross-module enum lookup.The change from
globals().get()to_get_scoped_enum_class()correctly implements the cross-module lookup mechanism. Theintfallback is appropriate sinceIntEnuminherits fromint, makingisinstance()checks still work correctly.
445-459: LGTM! Output conversion correctly uses cross-module lookup.The change from
globals().get()to_get_scoped_enum_class()with a lambda fallback maintains the original behavior while enabling cross-module enum resolution. The lambda fallback ensures that if the enum class isn't found, the int value is returned unchanged.
| | return _scoped_enum_registry[name] | ||
| | # Slow path: search all loaded pyopenms modules | ||
| | for mod_name, mod in list(_sys.modules.items()): | ||
| | if mod is not None and (mod_name.startswith('pyopenms.') or mod_name == 'pyopenms'): |
There was a problem hiding this comment.
Nope 😄 this will not work generally
There was a problem hiding this comment.
if only pyopenms uses it ;)
The existing test_cross_module_scoped_enum_imports test had a gap: it didn't test the scenario where a class in module B uses both getter and setter methods for an enum defined in module A. This is the exact pattern that failed in pyOpenMS: s.setType(s.getType()) # Cross-module roundtrip Changes: - Add StatusTracker class to EnumConsumer with getter/setter for enums from EnumProvider (both Task::TaskStatus and Priority) - Add Runtime Test 8 that validates tracker.setX(tracker.getX()) works correctly across module boundaries - Update test assertions to check for _get_scoped_enum_class() pattern instead of the old globals().get() pattern Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
When pyopenms splits the module, it knows which decls are where. Maybe one should fix it in the create_cpp_extension of pyopenms instead. To make sure it creates a set of modules without circular dependencies. Maybe we also need to rethink scoped enums. Maybe we need a real Cython implementation of them that behave like regular enums, such that we can do type checking on Cython level and dont need to import all python modules. |
Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
@timosachsenberg I dont think this was a good idea. See my comments. |
|
ups too late :) ok how can we fix this such that it works generally? |
|
See my comments. Maybe the AI has better ideas if it knows other places/ways where this could be solved. |
|
Fixed in a76f252 - the lookup is now package-agnostic. It auto-detects the package name at runtime using |
|
Not bad but in this case the user needs to make sure they are named in a specific way. We should document this somewhere. |
Summary
Fixes #239 - Cross-module enum class getter/setter inconsistency in multi-module builds.
In multi-module builds (e.g., pyOpenMS with
_pyopenms_1.pyxthrough_pyopenms_8.pyx), scoped enums (enum class) wrapped as Python IntEnum classes could fail when used across modules.Problem
The previous
globals().get()approach only searched the current module's namespace:MSSpectrum(in module 4) calledgetType()returningSpectrumType(defined in module 3), the getter returned a plainintbecause_PySpectrumTypewasn't in module 4's globalsAttributeError: 'int' object has no attribute 'value'because it expected the IntEnum wrapperSolution
Add a cross-module enum registry and lookup mechanism:
_scoped_enum_registry): A dict that stores enums defined in each module_get_scoped_enum_class):pyopenms.*modules viasys.modulestop_level_pyx_codeso it only appears in.pyxfiles, not.pxdfiles (which cannot containdefstatements)Changes
CodeGenerator.py:create_scoped_enum_helpers()methodimport sys as _systo default cimportscreate_foreign_enum_imports()ConversionProvider.py:type_check_expression()to use_get_scoped_enum_class()output_conversion()to use_get_scoped_enum_class()with lambda fallbackTest plan
s.setType(s.getType())now works for bothSpectrumSettings(same module) andMSSpectrum(cross-module)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.