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

[NEAT-236] DMS Exporter Support downloading Solution with Enterprise 🧑‍🧒 #448

Merged
merged 15 commits into from
May 13, 2024

Conversation

doctrino
Copy link
Collaborator

@doctrino doctrino commented May 11, 2024

Adds option for Importing solution model with enterprise, shown with the argument reference_model_id below.

from cognite.neat.rules.importers import DMSImporter

importer = DMSImporter.from_data_model_id(cognite_client, solutiom_model_id, reference_model_id)

rules = importer.to_rules()

It is tempting to allow for DMSImporter.from_data_model_id(cognite_client, solutiom_model, reference_model_id="infer"). However, I think we should follow the principle of being explicit here.

Note that this opens up for a new type of warning, you can have a reference model that is not referenced by the use model. (and potentially some other issues as well which I have not thought about). I have logged that in this task: https://cognitedata.atlassian.net/browse/NEAT-247.

I think one of the principles I realized working on this, is to avoid mixing the logic and validation if you can. If you are missing a data model, then it is an error an you have to return it as an error, but warnings like reference model not being referenced should be likely kept to the .validation method of the schema.

@doctrino doctrino changed the base branch from remove-unused to refactor-dms-schema-single-model May 11, 2024 07:41
@doctrino doctrino marked this pull request as ready for review May 12, 2024 06:26
@doctrino doctrino requested a review from a team as a code owner May 12, 2024 06:26
Copy link

github-actions bot commented May 12, 2024

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
19935 13553 68% 60% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
cognite/neat/_version.py 100% 🟢
cognite/neat/rules/importers/_dms2rules.py 79% 🟢
cognite/neat/rules/models/dms/_schema.py 87% 🟢
TOTAL 89% 🟢

updated for commit: 66751fc by action🐍

cls,
client: CogniteClient,
data_model_id: DataModelIdentifier,
reference_model_id: DataModelIdentifier | None = None,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that being explicit with stating reference_mode_id. Later on we can add magic of inference, but my worry is if that we might bump into data models not created by neat that might be derived from having more than one reference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an interesting discussion, will add it to tech debt

client: CogniteClient,
data_model_id: DataModelIdentifier,
reference_model_id: DataModelIdentifier | None = None,
) -> "DMSImporter":
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this method we only support getting the latest version. We could have mismatch that for some reason user data model was created on top of reference data model, which version is not latest.

So maybe add NOTE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you can specify whatever datamodel version you want.

DataModelIdentifier = tuple[str, str] | tuple[str, str, strr] | dm.DataModelingId

Base automatically changed from refactor-dms-schema-single-model to main May 13, 2024 11:53
@doctrino doctrino merged commit 54fb2ec into main May 13, 2024
7 checks passed
@doctrino doctrino deleted the dms-exporter-support-multi-model-download branch May 13, 2024 12:04
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