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

Update add_ml_model and add_script #304

Merged
merged 25 commits into from
Jun 28, 2023

Conversation

mellis13
Copy link
Contributor

@mellis13 mellis13 commented Jun 14, 2023

This PR addresses incorrect or inconsistent behavior in the Model.add_ml_model(), Model.add_script() and Model.add_function() API. The following changes are included in this PR:

  • Update documentation to be more accurate
  • Correct the code for setting models on multiple GPUs
  • Update tests so that they run on GPU and multiple GPUs
  • Change the default network interface for test to be lo instead of ipogif0

@mellis13 mellis13 requested a review from MattToast June 14, 2023 16:32
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #304 (af38806) into develop (03f2ba7) will decrease coverage by 0.49%.
The diff coverage is 66.66%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #304      +/-   ##
===========================================
- Coverage    87.44%   86.96%   -0.49%     
===========================================
  Files           59       59              
  Lines         3481     3482       +1     
===========================================
- Hits          3044     3028      -16     
- Misses         437      454      +17     
Impacted Files Coverage Δ
smartsim/entity/model.py 96.39% <ø> (ø)
smartsim/_core/config/config.py 98.41% <66.66%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Two quick requests tangentially related to this PR, but otherwise lgtm!!

smartsim/_core/entrypoints/colocated.py Show resolved Hide resolved
@@ -462,7 +468,9 @@ def add_function(
:type script_path: str, optional
Copy link
Member

Choose a reason for hiding this comment

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

Looks like :param script: and :param script_path: are copy/paste errors from back in the day. Can you update these while you are here

@mellis13 mellis13 marked this pull request as draft June 16, 2023 06:22
@mellis13
Copy link
Contributor Author

Thanks for the great feedback. I'm going to move this PR to "draft" because I was looking at the code more and see that the tests for these functions were not being tested on GPU for WLM tests when device was set to GPU. I'll address your feedback and a few other things and change the status back when it is ready for review.

@mellis13 mellis13 changed the title Fix mixing Model functions in API summary Update add_ml_model and add_script Jun 16, 2023
@mellis13 mellis13 marked this pull request as ready for review June 21, 2023 20:19
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

Looks great! A couple of small type errors, a couple quick changes to the tests, and couple of questions for follow up work outside of this PR, but otherwise looks good to me!!

conftest.py Outdated Show resolved Hide resolved
smartsim/_core/config/config.py Outdated Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
smartsim/entity/model.py Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
tests/backends/test_dbmodel.py Show resolved Hide resolved
tests/backends/test_dbscript.py Show resolved Hide resolved
Copy link
Member

@MattToast MattToast left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for the hard work on this one!!

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