-
Notifications
You must be signed in to change notification settings - Fork 2
MetaxyMetadataStoreResource #329
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
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage Report (Python 3.11)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (Python 3.10)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Coverage Report (Python 3.13)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ad2dbe3 to
e4e9d77
Compare
d723165 to
57e2f2c
Compare
57e2f2c to
9412541
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces Dagster integration for Metaxy by implementing a MetaxyMetadataStoreResource that wraps Metaxy metadata stores as Dagster resources. The implementation includes:
- A configurable resource class that supports fallback stores, config file loading, and auto-discovery
- Context manager support for resource lifecycle management
- Comprehensive validation for fallback store compatibility (hash algorithms and truncation lengths)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/metaxy/ext/dagster/resource.py |
Core implementation of MetaxyMetadataStoreResource with config loading, fallback store resolution, and validation |
src/metaxy/ext/dagster/__init__.py |
Module exports for the Dagster extension |
tests/ext/test_dagster.py |
Comprehensive test suite covering config loading, fallback store validation, and Dagster integration |
pyproject.toml |
Added dagster dependency to optional extras and dev dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """Context manager exit - closes the underlying store.""" | ||
| if hasattr(self, "_active_store") and self._active_store is not None: | ||
| self._active_store.__exit__(*args) | ||
| self._active_store = None # type: ignore[assignment] |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting _active_store to None conflicts with the type system since _active_store is expected to be a MetadataStore. Consider using del self._active_store instead to properly clean up the attribute, or define _active_store: MetadataStore | None as an explicit attribute on the class.
| if not hasattr(self, "_active_store") or self._active_store is None: | ||
| self._active_store = self.get_store() |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _active_store attribute is never declared on the class, which makes the code harder to understand and may cause issues with type checkers. Consider declaring it as a class attribute: _active_store: MetadataStore | None = None.
| if TYPE_CHECKING: | ||
| pass |
Copilot
AI
Nov 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty if TYPE_CHECKING: block serves no purpose. Remove this conditional or add the necessary type imports inside it.
| if TYPE_CHECKING: | |
| pass |

a very first rough draft
eventually resolves #159