Fix circular import issue with cross-module scoped enum type checking

[timosachsenberg]

Summary

PR #237 introduced module-level Python imports for scoped enum classes across modules. While this fixed the Cython compile-time error, it causes circular import errors at runtime when modules form import chains during initialization.

Problem

When autowrap generates multi-module builds (like pyOpenMS with _pyopenms_1 through _pyopenms_8), the modules import each other during initialization:

1. Module 1 starts loading and imports from Module 8
2. Module 8 imports from Module 7, etc.
3. When Module 2 tries to import _PyChecksumType from Module 3, Module 3 hasn't finished initializing yet

This results in:

ImportError: cannot import name _PyChecksumType

Solution

Instead of module-level imports, use globals().get() for late binding:

ConversionProvider.py - Modified type_check_expression():

# Before (from PR #237):
return "isinstance(%s, %s)" % (argument_var, name)

# After:
return "isinstance(%s, globals().get('%s', int))" % (argument_var, name)

CodeGenerator.py - Made create_foreign_enum_imports() a no-op with documentation explaining why.

This approach:

• ✅ Compiles successfully (globals().get() is always valid Python syntax)
• ✅ Resolves the enum class at runtime after all modules are fully loaded
• ✅ Falls back to int which works for IntEnum values (IntEnum inherits from int)
• ✅ Avoids circular import issues

Testing

• All 491 pyOpenMS unit tests pass
• All 82 static method tests pass (these specifically test enum conversion functions)
• Updated unit tests in test_code_generator.py to verify the new behavior

Related

• Fixes the runtime issue introduced by Fix cross-module enum class imports for type assertions #237
• See comment on Fix cross-module enum class imports for type assertions #237 for detailed analysis: Fix cross-module enum class imports for type assertions #237 (comment)

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Fixed circular import issues when using Enums across modules

• Refactor

  • Changed cross-module Enum import handling to use runtime binding instead of module-level imports
  • Updated type checking to support late-bound Enum resolution via runtime lookup

• Tests

  • Updated test suite to validate the new late-binding approach for cross-module Enum references

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR removes module-level import generation for cross-module scoped enums and replaces type checking and output conversion with late-binding via globals().get(), eliminating circular import dependencies by deferring enum class resolution to runtime.

Changes

Cohort / File(s)	Summary
Core enum binding refactoring autowrap/CodeGenerator.py, autowrap/ConversionProvider.py	Disables create_foreign_enum_imports to no-op; updates EnumConverter type checking and output conversion to use late-binding globals().get() with fallbacks instead of direct enum class references, avoiding circular imports.
Test updates tests/test_code_generator.py	Replaces import-generation assertions with no-op behavior verification; updates cross-module scoped enum tests to validate late-binding mechanism via globals().get() instead of module-level imports.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix cross-module enum class imports for type assertions #237: Directly addresses the same cross-module scoped-enum handling in CodeGenerator.create_foreign_enum_imports and EnumConverter, but proposes an opposing solution using explicit Python imports rather than late-binding.
• Add test for enum-based overload resolution #234: Modifies scoped-enum handling for overload resolution and isinstance logic; enum isinstance/overload tests may be affected by this PR's late-binding changes.

Suggested reviewers

• jpfeuffer
• poshul

Poem

🐰 Late bindings dance where imports once stood,
Circular threads untangled, as they should,
With globals().get() we softly bind,
Enum classes found at runtime's design,
No more import chaos—clean, refined! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title directly and clearly describes the main change: fixing a circular import issue with cross-module scoped enum type checking, which aligns with the primary objective of removing module-level imports and implementing late binding.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[timosachsenberg]

rabbit hole level 10 mittlerweile... @jpfeuffer hat das late binding Nachteile? damit hat pyopenms zumindest gebaut

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

autowrap/ConversionProvider.py (2)

405-435: Scoped-enum type checks now depend on local globals; confirm cross-module behavior

Switching scoped-enum checks to isinstance(arg, globals().get('Name', int)) avoids hard references and circular imports, but it also means that in a consuming module that does not bind the enum class into its own globals, the check silently degrades to isinstance(arg, int). In that case, any int or unrelated IntEnum instance will satisfy the assertion, so you lose the previous ability to distinguish between different IntEnum types at runtime in cross-module call sites. Please confirm that, for all intended consumers, the corresponding Python enum classes are guaranteed to be bound into that module’s globals (so the fallback to int is only a safety net and not the common path); otherwise this is a real weakening of type validation that might make tests like the “wrong enum type should raise” expectations in test_cross_module_scoped_enum_imports impossible to satisfy reliably. A more robust (but still late-bound) alternative would be to resolve the enum from its defining module via sys.modules and getattr, falling back to int only if that also fails.

---

456-463: Scoped-enum output conversion may return plain ints in cross-module consumers

The new output_conversion for scoped enums wraps the integer value via globals().get('Name', lambda x: x)(<int>_r), which is fine when the enum class is in the current module’s globals, but in cross-module wrappers that don’t expose the enum symbol this will quietly return a bare int instead of an enum instance. That’s likely acceptable given the IntEnum–int compatibility and matches your comment, but it’s worth calling out as an intentional behavioral change: downstream code can no longer rely on isinstance(result, EnumClass) across modules unless EnumClass is explicitly re-exported into the consumer module.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e37b334 and b7ef998.

📒 Files selected for processing (3)

• autowrap/CodeGenerator.py
• autowrap/ConversionProvider.py
• tests/test_code_generator.py

⏰ 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.1.0, 3.13)
• GitHub Check: test (==3.1.0, 3.11)
• GitHub Check: test (==3.2.0, 3.12)
• GitHub Check: test (==3.1.0, 3.12)
• GitHub Check: test (==3.1.0, 3.10)
• GitHub Check: test (==3.2.0, 3.11)
• GitHub Check: test (==3.2.0, 3.10)

🔇 Additional comments (3)

autowrap/CodeGenerator.py (1)

2041-2077: create_foreign_enum_imports no-op is clearly documented and consistent

The function is now an explicit, documented no-op, which cleanly centralizes enum import behavior into the converters while avoiding circular imports. Keeping the call site but making the implementation inert is reasonable for backward compatibility.

tests/test_code_generator.py (2)

645-747: Test correctly pins create_foreign_enum_imports as a no-op

This test now explicitly asserts that create_foreign_enum_imports() produces no Python-level imports, which is exactly the new contract and guards against reintroducing circular imports in future changes. Scanning top_level_pyx_code is the right place to enforce this.

---

752-979: Cross-module scoped-enum test nicely validates late binding, but depends on enum globals being present

The integration test does a good job of pinning the new behavior: it forbids module-level enum imports in EnumConsumer.pyx, requires the globals().get(...) pattern, and then exercises real cross-module usage at runtime. One subtle dependency is that the negative checks (runner.isHighPriority(TaskStatus.PENDING) etc. expecting AssertionError) only hold if the relevant enum classes are actually bound into EnumConsumer’s globals so that globals().get('...') resolves to the enum type rather than falling back to int as per the updated EnumConverter. It’s worth double-checking that the codegen path does establish these bindings for all such enums; otherwise those assertions will start silently accepting mismatched IntEnums.

[jpfeuffer]

Hmmm not 100% sure but it looks like it is kind of ok although very ugly.
The disadvantage is of course that it hides dependencies but we don't care. In theory it IS a single module. We only split because of compile time.

I am actually incredibly surprised that we did not have a circular import yet. Ask the AI how/why this works for normal classes. I bet we have normal classes that span module boundaries. Do we do the type check on Cython/C class level? I don't think so.

The only correct solution would be to split openms instead and make a real dependency tree (which I planned for a long time already anyway). I.e. not split into N equal parts "randomly". But not sure if we can split OpenMS enough to reach that level of modularity to compile so fast.

[jpfeuffer]

https://dev.to/aaron_rose_0787cc8b4775a0/the-circular-import-problem-breaking-dependency-cycles-4i56

Good read

[timosachsenberg]

ow I can explain the key difference. The issue is specific to scoped enums because of how they're wrapped versus other types:

Different Type Checking Approaches
┌────────────────┬────────────────────────┬────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Type │ Type Check │ Why No Import Issue │
├────────────────┼────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Regular │ isinstance(x, │ ClassName is a Cython extension type (cdef class) - it's resolved at the C level during Cython │
│ classes │ ClassName) │ compilation, not via Python imports │
├────────────────┼────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Non-scoped │ x in [0, 1, 2, ...] │ Just checks if value is in a list of integers - no class reference needed │
│ enums │ │ │
├────────────────┼────────────────────────┼────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Scoped enums │ isinstance(x, │ _PyEnumName is a Python IntEnum class - a pure Python object that must be imported │
│ │ _PyEnumName) │ │
└────────────────┴────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────┘
The Root Cause

From ConversionProvider.py:586-587 (regular classes):
def type_check_expression(self, cpp_type: CppType, argument_var: str) -> str:
return "isinstance(%s, %s)" % (argument_var, cpp_type.base_type)

This looks similar to scoped enums, but the crucial difference is that cpp_type.base_type here refers to a Cython cdef class - these are C extension types that exist at the Cython/C level and don't require Python module imports.

From ConversionProvider.py:406-408 (non-scoped enums):
if not self.enum.scoped:
values = ", ".join(str(v) for (__, v) in self.enum.items)
return "%s in [%s]" % (argument_var, values)

Non-scoped enums just check if the integer value is valid - no class reference at all.

Scoped enums are unique because they're wrapped as IntEnum Python classes (for better Python ergonomics), and these are pure Python objects that need to be imported across modules - triggering the circular import problem when modules depend on each other during initialization.

Scoped enums are the only case where autowrap generates code that references a Python class (IntEnum) defined in another autowrap module. Everything else either:

• Uses C-level Cython types (resolved via cimport at compile time)
• Uses Python built-in types (always available)
• Uses integer value checks (no class reference)

[jpfeuffer]

Interesting but how does this work? The input to the function in the pyx is a python object, e.g. MSExperiment. How does isinstance(%s, %s)" % (argument_var, cpp_type.base_type) work in this case?? base_type probably is _MSExperiment (the C type then) wouldn't it need to get argument_var.inst.get() to check then?

[timosachsenberg]

@jpfeuffer Good question! The key insight is that there are two different names for each wrapped class:

Name	What it is	Used for
MSExperiment	cdef class (Python wrapper)	isinstance() checks, Python-level code
_MSExperiment	C++ type (from cimport)	C-level declarations like shared_ptr[_MSExperiment]

Looking at generated code (e.g., libcpp_test.pyx):

# C++ type imported with underscore prefix
from libcpp_test cimport Int as _Int

# Python wrapper class (same name, no underscore)
cdef class Int:
    cdef shared_ptr[_Int] inst
    ...

# isinstance check uses the cdef class name (no underscore)
assert isinstance(i, Int), 'arg i wrong type'

So cpp_type.base_type is "MSExperiment" (no underscore), which matches the cdef class MSExperiment name. The underscore-prefixed _MSExperiment is only used for internal C++ type references.

Why no .inst.get() is needed:

• We're not checking the C++ type (_MSExperiment)
• We're checking the Python wrapper type (MSExperiment)
• The argument IS already an instance of cdef class MSExperiment
• cdef class types are valid Python types (extension types backed by PyTypeObject)

Why scoped enums are different:
Scoped enums are wrapped as pure Python IntEnum classes (not cdef class), which need Python-level imports (not cimport) and hence cause the circular import issues that this PR fixes.