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.

Sign up for GitHub

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

Jump to bottom

Add UT for Model #1229

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Add UT for Model #1229

merged 1 commit into from
Nov 3, 2023

Conversation

jieguangzhou
Copy link
Collaborator

@jieguangzhou jieguangzhou commented Nov 2, 2023 •
edited
Loading

Description

#1172

Related Issues

Checklist

  • Is this code covered by new or existing unit tests or integration tests?
  • Did you run make test successfully?
  • Do new classes, functions, methods and parameters all have docstrings?
  • Were existing docstrings updated, if necessary?
  • Was external documentation updated, if necessary?

Additional Notes or Comments

@jieguangzhou jieguangzhou linked an issue Nov 2, 2023 that may be closed by this pull request
Unittests for Model #1172
Closed
@jieguangzhou jieguangzhou self-assigned this Nov 2, 2023
jieguangzhou
jieguangzhou commented Nov 2, 2023
View reviewed changes
superduperdb/components/model.py
@@ -54,7 +54,7 @@ def get(self, k, default=None):
try:
return getattr(self, k)
except AttributeError:
return self.kwargs.get(k, default=default)
return self.kwargs.get(k, default)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dict.get does not havedefault parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha.

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (34830a7) 80.33% compared to head (d88b1a6) 80.87%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1229      +/-   ##
==========================================
+ Coverage   80.33%   80.87%   +0.53%     
==========================================
  Files          95       95              
  Lines        6602     6605       +3     
==========================================
+ Hits         5304     5342      +38     
+ Misses       1298     1263      -35
Files Coverage Δ
superduperdb/components/model.py 93.66% <100.00%> (+11.00%) ⬆️
superduperdb/base/logger.py 86.95% <80.00%> (-3.05%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jieguangzhou jieguangzhou force-pushed the unittests/model branch 4 times, most recently from 9884d69 to 8178285 Compare November 3, 2023 04:17
@jieguangzhou jieguangzhou marked this pull request as ready for review November 3, 2023 06:20
@jieguangzhou
Copy link
Collaborator Author

I mocked out DB and Select related classes, so in some test cases, the model will not actually interact with the DB.
And just test the logic of functions in the PredictMixin and Model.

@jieguangzhou
Copy link
Collaborator Author

Later, UT will be added for fit and predict of different AI frameworks in ext.

@jieguangzhou jieguangzhou force-pushed the unittests/model branch 2 times, most recently from a4c045d to d88b1a6 Compare November 3, 2023 09:30
blythed
blythed approved these changes Nov 3, 2023
View reviewed changes
test/unittest/component/test_model.py
expect = postprocess(to_call(X))
assert np.allclose(predict_mixin._predict_one(X), expect)

# Bad postprocess
Copy link
Collaborator

Choose a reason for hiding this comment

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

What type of bad postprocess?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

        if isinstance(self.postprocess, Artifact):
            outputs = [self.postprocess.artifact(o) for o in outputs]
        elif self.postprocess is not None:
            raise ValueError('Bad postprocess')

postprocess must be a Arfifact or None

test/unittest/component/test_model.py


@patch.object(Datalayer, 'add')
def test_pm_predict_and_listen(mock_add, predict_mixin, local_empty_db):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice.

test/unittest/component/test_model.py
db.databackend.id_field = 'id_field'

# overwrite = True
with patch.object(predict_mixin, '_predict_with_select_and_ids') as mock_predict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice -- yes this is needed (important for efficiency).

superduperdb/components/model.py
@@ -54,7 +54,7 @@ def get(self, k, default=None):
try:
return getattr(self, k)
except AttributeError:
return self.kwargs.get(k, default=default)
return self.kwargs.get(k, default)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha.

@jieguangzhou jieguangzhou merged commit 38b0130 into superduper-io:main Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blythed blythed blythed approved these changes

@thejumpman2323 thejumpman2323 Awaiting requested review from thejumpman2323

Assignees

@jieguangzhou jieguangzhou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unittests for Model
3 participants
@jieguangzhou @codecov-commenter @blythed