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

Add file storage for SQLite. Add integration tests. #55

Merged
merged 3 commits into from
Jun 7, 2020
Merged

Add file storage for SQLite. Add integration tests. #55

merged 3 commits into from
Jun 7, 2020

Conversation

dcostea
Copy link
Collaborator

@dcostea dcostea commented Jun 6, 2020

Resolves

#34 #51

Description

I have added two integration tests (one for creating an experiment in sqlite db and another one for uploading a model file)

Copy link
Owner

@aslotte aslotte left a comment

Choose a reason for hiding this comment

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

@dcostea this is great work, thank you for tackling this so both the Azure and SQLite implementation are up to par. I added a couple of minor comments.

src/MLOps.NET.SQLite/Storage/LocalFileModelRepository.cs Outdated Show resolved Hide resolved
public void CreateExperimentAsync_Success()
{
//Arrange
MLLifeCycleManager mlm = new MLLifeCycleManager();
Copy link
Owner

Choose a reason for hiding this comment

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

Super awesome that you're adding the first tests! For a convention, I think it would be nice if we call the class we are testing either classUnderTest or unitUnderTest, I'll let you decide between those two :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this is what you meant, I put CreateExperimentAsync_Always_ReturnsNonEmptyGuid and UploadModelAsync_ValidModelPath_UploadSuccess

@aslotte aslotte mentioned this pull request Jun 7, 2020
@@ -4,10 +4,10 @@ namespace MLOps.NET.SQLite
{
public static class MLLifeCycleManagerExtensions
{
public static MLLifeCycleManager UseSQLite(this MLLifeCycleManager mLLifeCycleManager)
public static MLLifeCycleManager UseSQLite(this MLLifeCycleManager mLLifeCycleManager, string destrinationFolder)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
public static MLLifeCycleManager UseSQLite(this MLLifeCycleManager mLLifeCycleManager, string destrinationFolder)
public static MLLifeCycleManager UseSQLite(this MLLifeCycleManager mLLifeCycleManager, string destrinationFolder = "C:\MLOps")

Copy link
Owner

@aslotte aslotte left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aslotte aslotte merged commit 695ba51 into aslotte:master Jun 7, 2020
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