Skip to content

Resolve import loops#301

Closed
EarlMilktea wants to merge 5 commits intomasterfrom
resolve-loop
Closed

Resolve import loops#301
EarlMilktea wants to merge 5 commits intomasterfrom
resolve-loop

Conversation

@EarlMilktea
Copy link
Contributor

@EarlMilktea EarlMilktea commented Jun 12, 2025

Context (if applicable):

This PR fixes another import loop causing problems in #280 .

Description of the change:

  • Resolve import loops caused by prety_print.py
  • Enforce top-level imports by ruff
    • Some exceptions exist: see pyproject.toml .

After merging this PR, for all the typed files, import will become completely loop-free.

loops-pruned

@codecov
Copy link

codecov bot commented Jun 12, 2025

Codecov Report

Attention: Patch coverage is 96.51163% with 3 lines in your changes missing coverage. Please review.

Project coverage is 79.82%. Comparing base (1dc02d7) to head (28ad8cf).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
graphix/command.py 96.92% 2 Missing ⚠️
graphix/pattern.py 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #301      +/-   ##
==========================================
- Coverage   79.83%   79.82%   -0.01%     
==========================================
  Files          36       36              
  Lines        5429     5428       -1     
==========================================
- Hits         4334     4333       -1     
  Misses       1095     1095              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@EarlMilktea EarlMilktea self-assigned this Jun 12, 2025
@EarlMilktea EarlMilktea marked this pull request as ready for review June 12, 2025 17:26
@thierry-martinez
Copy link
Collaborator

I would be very disappointed if pattern_to_str could not remain in pretty_print.py (and, to a lesser extent, the same applies to command_to_str):

  • We should reduce the size of the pattern.py module.

  • I would like to clearly separate data structure definitions (patterns, commands) from algorithmic operations (pretty-printing, optimization, simulation, visualization), with each operation having its own dedicated module.

@EarlMilktea
Copy link
Contributor Author

@thierry-martinez

I really understand your opinion but still import loop is what we should not have.
It is ridiculous that I see esoteric errors each time I change the order of import .

clearly separate data structure definitions from algorithmic operations
pattern_to_str remain in pretty_print.py

This could be possible if Pattern is downsized. I'm happy with suspending this PR until ready.

command_to_str

This is 100 % impossible since it immediately results in import loop.

Let's find our point of compromise.

thierry-martinez added a commit to thierry-martinez/graphix that referenced this pull request Jun 20, 2025
This commit proposes a lighter fix than TeamGraphix#301 to resolve the import
loop between `pretty_print` and `command`.
thierry-martinez added a commit to thierry-martinez/graphix that referenced this pull request Jun 20, 2025
This commit introduces a lighter fix than TeamGraphix#301 to resolve the import loop between `pretty_print` and `command`.

The import loop originated from:
- In `command.py`: `from graphix.pretty_print import DataclassPrettyPrintMixin`
- In `pretty_print.py`: `from graphix import command`

If `pretty_print.py` was loaded first, then the statement `from graphix import command` would trigger the  loading of `command.py` before `pretty_print` was fully initialized. As a result, the import `from graphix.pretty_print import DataclassPrettyPrintMixin` would fail
because `DataclassPrettyPrintMixin` had not yet been defined.

This commit resolves the issue by replacing the import in `command.py` with `from graphix import pretty_print`. This allows the two modules to depend on each other without causing a cyclic import failure.
@thierry-martinez
Copy link
Collaborator

I agree with you that we should consider import loops to be bugs, and we want to resolve them so that modules can be imported in any order without causing random failures.

I propose in #304 a lighter fix to resolve the import loop between pretty_print and command. In this PR, you also move the statement from graphix.sim.density_matrix import DensityMatrix to the top level of random_objects.py. That seems correct, and I can’t explain why I felt the need to move it elsewhere in commit a9493b7, since it did not appear to be necessary, so we can keep your fix for this in the PR.

@EarlMilktea
Copy link
Contributor Author

@thierry-martinez

Checked #304 .

While it would be sufficient for now, I still strongly believe that, at least in the future, we need to consider loop-free (DAG) import structure and actually this is possible by transforming xxx_to_str to methods.
I admit your design (separate data structure definitions from algorithmic operations) is very common and idiomatic in some cases (for example as Rust does), but specifically for Python, class-based encapsulation (implement class functionalities locally inside the class) is also equally idiomatic and this time clearly superior to yours as it forever eliminates subtle import-order errors.

I hope we someday reach agreement.

@thierry-martinez
Copy link
Collaborator

Python doesn't forbid cycles in the dependency graph, only import-time execution cycles are problematic. Moreover, the key issue isn’t whether something is implemented inside a class or a function: object-oriented design can be helpful in many cases.

The deeper problem lies in having a large, central module like pattern, which is inherently core to graphix. This makes it tempting to place a lot of functionality there. But that’s a common pitfall when aiming for a strict DAG design: it tends to create “god modules” that accumulate too much responsibility. Instead, I believe we should aim for better separation of concerns between distinct functionalities.

Conversely, naive object-oriented design can also lead to fragmentation: spreading the implementation of a single functionality across multiple classes or modules. Grouping related algorithmic parts can actually improve cohesion and clarity. For example, having a dedicated pretty_print module that sets the conventions for rendering both patterns and commands helps ensure consistency and improves maintainability. In that sense, keeping the formatting logic centralized is a benefit, not a flaw.

@EarlMilktea
Copy link
Contributor Author

This could be possible if Pattern is downsized. I'm happy with suspending this PR until ready.

As I said I'm ready for delaying this PR so that Pattern does not becomes too "divine" now.
What I want to confirm here is, we should someday ensure loop-free import structures statically by PLC0415 , so that this kind of random failures never occur in the future.

thierry-martinez added a commit to thierry-martinez/graphix that referenced this pull request Jun 23, 2025
This commit enables ruff rule PLC0415.

Suggested by EarlMilktea: TeamGraphix#301 (comment)
@thierry-martinez
Copy link
Collaborator

The rule PLC0415 could be desirable (and we can easily comply with it; see #307), but it does not require a DAG structure. As I said, only import-time execution cycles are problematic, and I think that requiring a DAG structure will reduce the code’s modularity and maintainability by forcing things to be grouped or separated for purely technical reasons, regardless of the logic of the code.

@EarlMilktea
Copy link
Contributor Author

will reduce the code’s modularity and maintainability by forcing things to be grouped or separated for purely technical reasons, regardless of the logic of the code

I think, it is still preferable to random breakdowns.

@thierry-martinez
Copy link
Collaborator

We agree that import-time execution cycles should be considered as bugs, and we want to resolve them so that modules can be imported in any order without causing random failures. PR #304 resolves a bug preventing graphix.pretty_print from being imported first. PR #307 makes the whole code base satisfy PLC0415 by moving all imports to the top of modules. Do you approve these PR?

@EarlMilktea EarlMilktea closed this Jul 3, 2025
thierry-martinez added a commit that referenced this pull request Jul 5, 2025
* Enable ruff rule PLC0415

This commit enables ruff rule PLC0415.

Suggested by EarlMilktea: #301 (comment)

Introduce `repr_mixins` for `DataclassReprMixin` and `EnumReprMixin`.
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.

2 participants