Skip to content
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

Suggestion: Remove circular dependencies #376

Open
jcapriot opened this issue Mar 1, 2025 · 5 comments
Open

Suggestion: Remove circular dependencies #376

jcapriot opened this issue Mar 1, 2025 · 5 comments

Comments

@jcapriot
Copy link
Member

jcapriot commented Mar 1, 2025

Both packages Aurora and mtpy-v2 list each other as runtime requirements. While this is not explicitly disallowed in python packaging, it can lead to some installation struggles, and potential circular imports, thus me marking this as a suggestion.

My suggestion is to better define what each package attempts to handle. My suspicion is that mtpy-v2 should depend on aurora, and not the other way around. Likely using the underlying mth5 and/or mt_metadata as the dedicated exchange format between the two.

I’m happy to help resolve any dependencies, or help refactor a bit of code, as long as there’s a clear description of what each package is meant to do.

I’ll open a matching PR on mtpy-v2

@kkappler
Copy link
Collaborator

kkappler commented Mar 8, 2025

Aurora is intended to show a pathway to generating mt_metadata formatted transfer functions from the mth5 data format. Because aurora is an example implementation, it makes sense to add it under mtpy-v2, both so it can be used for processing, as well as to serve as one example of how to plug things in with mt_metadata and mth5 for processing.

Up until recently there were no circular imports, however, a concept that were developed in aurora, namely the idea of a KernelDataset class that describes an mth5 dataset that can be processed into a TF, which was "promoted" out of aurora and placed into mtpy-v2. It is the need to import this class back into aurora that was causing can lead to a circular import.

I have wrapped a try/except around the offending block for now, on the currently in development features branches.

@jcapriot
Copy link
Member Author

jcapriot commented Mar 9, 2025

Would it make sense to move KernelDataset to mth5, or to make the KernelDataset in mtpy-v2 a subclass of the KernelDataset in aurora?

@kkappler
Copy link
Collaborator

@kujaku11 what do you think about @jcapriot's suggestion? Since the processing stuffs in mtpy-v2 are intended to be used with mth5, this might be a clean way to handle this ... perhaps add a processing folder to mth5 with the fundamental blocks like KernelDataset, which of course, mtpy-v2 can then import and can aurora. I like it at first glance.

@kujaku11
Copy link
Collaborator

@jcapriot @kkappler I think we could probably move KernelDataset to mth5 because it's basically just an organizer of mth5 files.

I guess I had thought of the workflow of: data in mth5, mtpy organizes the data to be processed, and then sends it to a processing software like aurora.

I think the circular import might be an easy fix cause but I'll have a closer look. We should probably clean up dependencies anyway.

@kujaku11
Copy link
Collaborator

@kkappler @jcapriot Upon further review, I think you guys are correct. The best option would be to put RunSummary and KernelDataset into mth5. We could have a processing module from which to import from. That way any processing code can easily access these two key objects without being specific to aurora or mtpy-v2. I'll start create a branch in mth5 called processing and start to port over RunSummary and KernelDataset. It will then be up to the user to pick the processing code and configuration for processing. Sound good?

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

No branches or pull requests

3 participants