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

Refactoring the MLOpsDbContext #196

Merged
merged 9 commits into from
Jul 11, 2020
Merged

Refactoring the MLOpsDbContext #196

merged 9 commits into from
Jul 11, 2020

Conversation

aslotte
Copy link
Owner

@aslotte aslotte commented Jul 11, 2020

Resolves

Fixes #185

Description

What have we done?

  • Refactored to not use interfaces and instead rely directly on the entities (simpler)
  • Refactored to have separate func passed in to configure the entity maps. This is good as our CosmosDb provider need to have each entity in its own container. By default they all end up in the same which makes it really really slow
  • Added additional integration tests to ensure the Run is populated with navigation properties

Challenges
I wanted to initially refactor use the Include keyword for eager loading. This doesn't seem to be fully supported in Cosmos DB for EF Core, and as I would rather want us to have a shared implementation for our storage providers, we'll just have to manually build our entity maps if we need to. That comes with a slight performance degradation and a bit more verbose code, but something I'm okay with right now. It looks like Include will be supported in EF 5 with .NET 5, so we should be able to refactor this then.

@aslotte aslotte self-assigned this Jul 11, 2020
@aslotte
Copy link
Owner Author

aslotte commented Jul 11, 2020

@Brett-Parker / @AnoojNair this is ready for a review. It's a lot of changes, but in general it will speed up our Cosmos implementation, enforce better DB constraints and make it possible for us to more easily configure how our entity maps looks for each storage provider. If we need to support another storage provider currently not supported by EF Core we can do so, we'll just have to implement IMetaDataStore and return a mapped entity. There shouldn't be anything EF specific in those entities, thus I removed the interfaces.

Copy link
Collaborator

@Brett-Parker Brett-Parker left a comment

Choose a reason for hiding this comment

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

Looks good, most of this was done on stream. I think all the main storage providers are currently covered. Any future ones can be decided as required. Good work!

@aslotte
Copy link
Owner Author

aslotte commented Jul 11, 2020

Thanks @Brett-Parker! I made a final change now when I've fully understood how the CosmosDB provider works. Entities that we don't access directly from the DbContext (MLOpsDbContext in our case) can be embedded, our owned). If they are embedded, then include work for both non-relational and relational queries. As DataSchema and DataColumn don't have a value by themselves, but rather as part of a Data instance, I've configured them to be embedded and as such was able to simplify this query.

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.

Refactor MLOpsDbContext
2 participants