modeldb
modeldb copied to clipboard
VertaAI
[VUMM-297] Move artifact store to common module
Impact and Context
There was a old PR: https://github.com/VertaAI/modeldb/pull/2476 but it is too old and take more time to resolved merge conflicts and make it working so i have create this PR from latest code with some better code improvement.
Risks and Area of Effect
Testing
How to Revert
@jkwatson-verta I have already added many test cases but it is not feasible to run it with automation so I have disabled it or either not added in test suits. We should fix that in separate PR with a separate ticket.
Test cases:
- https://github.com/VertaAI/modeldb/blob/96d104b2bf2737989fe729f7828a08fe6bea2bf0/backend/src/test/java/ai/verta/modeldb/ExperimentRunTest.java#L4542
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/ArtifactStoreTest.java
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/NFSArtifactStoreTest.java
This looks generally ok. If we want to save testing until later, that's ok, but we should create an issue to track it.
This PR is gigantic, and to my eyes, includes functional changes with no tests for them. I don't want to merge this without testing on the new functionality.
This PR is gigantic, and to my eyes, includes functional changes with no tests for them. I don't want to merge this without testing on the new functionality.
https://github.com/VertaAI/modeldb/pull/3003#issuecomment-1201160949
@jkwatson-verta I have already added many test cases but it is not feasible to run it with automation so I have disabled it or either not added in test suits. We should fix that in separate PR with a separate ticket.
Test cases:
- https://github.com/VertaAI/modeldb/blob/96d104b2bf2737989fe729f7828a08fe6bea2bf0/backend/src/test/java/ai/verta/modeldb/ExperimentRunTest.java#L4542
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/ArtifactStoreTest.java
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/NFSArtifactStoreTest.java
@jkwatson-verta I have already added many test cases but it is not feasible to run it with automation so I have disabled it or either not added in test suits. We should fix that in separate PR with a separate ticket.
Test cases:
- https://github.com/VertaAI/modeldb/blob/96d104b2bf2737989fe729f7828a08fe6bea2bf0/backend/src/test/java/ai/verta/modeldb/ExperimentRunTest.java#L4542
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/ArtifactStoreTest.java
- https://github.com/VertaAI/modeldb/blob/main/backend/src/test/java/ai/verta/modeldb/NFSArtifactStoreTest.java
ok. we need to get modeldb tests running with automation as soon as possible, in my opinion. @eheinlein-verta can we prioritize this work?
Since the Jira ticket has no reason as to why this change is being made, can you add some description to the PR itself, so I can understand why this huge change is necessary?
Since the Jira ticket has no reason as to why this change is being made, can you add some description to the PR itself, so I can understand why this huge change is necessary?
Added the following comment in the description.
We have an artifact store with two different options which are S3 and NFS. so for that, we have created two services using spring boot to serve artifact store functionality. In the current situation, This functionality is used by two backends which are Registry and ModelDB and both the backends have their own code so whenever we have updated something in one place we have to manage it on the other backends as well. so to protect duplication and uniqueness of the code we need these changes in a common place so that both backends are used same code and we can manage the code maintainability from one place.
When this is merged, will it break any of the other service repositories?
Yes it will break the registry backend. We have one registry PR related to this.
Open new PR with the latest main code where protos changes are not required as per @conradoverta suggestion https://vertaai.slack.com/archives/C0306V04EJC/p1661487421380299?thread_ts=1661422409.689719&cid=C0306V04EJC
New PR: https://github.com/VertaAI/modeldb/pull/3153