fix: resolve circular imports and add CI enforcement#4277
Open
fzoll wants to merge 4 commits intoembeddings-benchmark:mainfrom
Open
fix: resolve circular imports and add CI enforcement#4277fzoll wants to merge 4 commits intoembeddings-benchmark:mainfrom
fzoll wants to merge 4 commits intoembeddings-benchmark:mainfrom
Conversation
KennethEnevoldsen
requested changes
Mar 22, 2026
Contributor
KennethEnevoldsen
left a comment
KennethEnevoldsen
left a comment
There was a problem hiding this comment.
I think we can simplify the imports to keep them at top-level
Contributor
|
I have also removed the "closes" for the issue as we still have a lot of functions that are moved inside the function, which is not ideal |
fzoll
force-pushed
the
fix/circular-imports
branch
from
a2acf7d to
4ff7dcc
Compare
Contributor
Author
|
@KennethEnevoldsen Kept as inline imports (circular dependency chains make top-level impossible):
|
Contributor
KennethEnevoldsen
left a comment
KennethEnevoldsen
left a comment
There was a problem hiding this comment.
Looks good but it seems like we reintroduced a circular import:
ImportError: cannot import name 'AbsTask' from partially initialized module 'mteb.abstasks' (most likely due to a circular import) (D:\a\mteb\mteb\mteb\abstasks_init_.py)
Samoed
reviewed
Mar 23, 2026
fzoll
force-pushed
the
fix/circular-imports
branch
2 times, most recently
from
4fab241 to
5274ab5
Compare
Member
|
Here you've also added #4216 |
Move top-level `import mteb` statements that created circular dependency chains through `mteb/__init__.py` into function bodies or replace with direct submodule imports. This eliminates 38 circular import chains. Files fixed: - task_metadata.py: defer `import mteb` to method body - cache.py: defer `import mteb` to function bodies - _create_table.py: replace `import mteb` with direct import from mteb.models.get_model_meta - deprecated_evaluator.py: defer `import mteb` to method body - abs_encoder.py: defer `import mteb` to method bodies - task_result.py: import TaskMetadata from submodule directly, defer get_task import to method body Add scripts/check_circular_imports.py that detects runtime circular imports via AST analysis and integrate it into both `make lint` and `make lint-check` targets so CI prevents future regressions. Closes embeddings-benchmark#3937
…ircular Move imports from mteb.get_tasks, mteb.models.get_model_meta, and mteb.benchmarks.get_benchmark to top-level in files where it doesn't cause circular dependencies (abs_encoder, cache, deprecated_evaluator). Keep inline imports only in task_metadata.py and task_result.py where circular dependency chains make top-level imports impossible. Replace `import mteb` with direct submodule imports throughout.
Remove scripts/check_circular_imports.py and its Makefile references per reviewer consensus. Move get_task import in abs_encoder.py to function-level to break the circular import chain: mteb.abstasks → abstask → mteb.models → abs_encoder → get_tasks → mteb.abstasks
fzoll
force-pushed
the
fix/circular-imports
branch
from
5274ab5 to
968dee9
Compare
Contributor
Author
|
@KennethEnevoldsen @Samoed Yeah, sorry again, the solution of #4216 is in my main (mistakenly), so i have to take extra care. |
The import was changed from mteb.get_model_metas to a direct import in mteb.cache, so the mock needs to patch mteb.cache.get_model_metas.
| patch.object(cache, "download_from_remote") as mock_download, | ||
| patch.object(cache, "load_results") as mock_load_results, | ||
| patch("mteb.get_model_metas") as mock_get_model_metas, | ||
| patch("mteb.cache.get_model_metas") as mock_get_model_metas, |
Contributor
There was a problem hiding this comment.
this seems like the wrong path
Suggested change
| patch("mteb.cache.get_model_metas") as mock_get_model_metas, | |
| patch("mteb.models.get_model_metas") as mock_get_model_metas, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mtebpackage by moving top-levelimport mtebstatements into function bodies or replacing them with direct submodule importsscripts/check_circular_imports.py— an AST-based detector for runtime circular imports (no new dependencies)make lintandmake lint-checkso CI prevents future regressionsWhat changed
mteb/abstasks/task_metadata.pyimport mtebto method bodymteb/cache.pyimport mtebto function bodiesmteb/benchmarks/_create_table.pyimport mtebwith directfrom mteb.models.get_model_meta import get_model_metamteb/deprecated_evaluator.pyimport mtebto method bodymteb/models/abs_encoder.pyimport mtebto method bodiesmteb/results/task_result.pyTaskMetadatafrom submodule directly; deferget_taskimportThe root cause in all cases was
import mtebat the top level of modules that are themselves imported bymteb/__init__.py, creating re-entrant import chains. Python handles these by returning a partially-initialized module, which works but is fragile — any change to import ordering could causeAttributeErrorat import time.Context
Addresses #3937 (original suggestion by @KennethEnevoldsen in PR #3800 review).