You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@submarine.apache.org by GitBox <gi...@apache.org> on 2021/06/01 04:52:33 UTC
[GitHub] [submarine] featherchen opened a new pull request #597: Submarine 822. Expand log_model for more package
featherchen opened a new pull request #597:
URL: https://github.com/apache/submarine/pull/597
### What is this PR for?
<!-- A few sentences describing the overall goals of the pull request's commits.
First time? Check out the contributing guide - https://submarine.apache.org/contribution/contributions.html
-->
Expand log_model for more package (keras and sklearn)
### What type of PR is it?
[Improvement ]
### Todos
* [ ] - Task
### What is the Jira issue?
<!-- * Open an issue on Jira https://issues.apache.org/jira/browse/SUBMARINE/
* Put link here, and add [SUBMARINE-*Jira number*] in PR title, eg. `SUBMARINE-23. PR title`
-->
https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-822
### How should this be tested?
<!--
* First time? Setup Travis CI as described on https://submarine.apache.org/contribution/contributions.html#continuous-integration
* Strongly recommended: add automated unit tests for any new or changed behavior
* Outline any manual steps to test the PR here.
-->
### Screenshots (if appropriate)
### Questions:
* Do the license files need updating? No
* Are there breaking changes for older versions? No
* Does this need new documentation? No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] featherchen commented on a change in pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
featherchen commented on a change in pull request #597:
URL: https://github.com/apache/submarine/pull/597#discussion_r646234917
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model.py
##########
@@ -54,7 +63,7 @@ def test_update_model(self, mocker):
def test_load_model(self, mocker):
mock_method = mocker.patch.object(mlflow.pyfunc, "load_model")
mock_method.return_value = mlflow.pytorch._PyTorchWrapper(
- LinearNNModel())
+ LinearNNModelTorch())
Review comment:
In this PR,I only deal with log_model function.By finitshed the expansion of this funtion,I think it will be easier to deal with other funtion(ex load_model) either in the same way or in Qlib's way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu commented on a change in pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu commented on a change in pull request #597:
URL: https://github.com/apache/submarine/pull/597#discussion_r644019766
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model.py
##########
@@ -33,10 +36,16 @@ def tearDown(self):
pass
@pytest.mark.skip(reason="Developing")
- def test_log_model(self, mocker):
+ def test_log_model(self, mocker, package_type):
Review comment:
This test is problematic. `package_type` would be None. You should separate different package types into different unit tests.
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model_e2e.py
##########
@@ -35,7 +35,7 @@ def models_client_fixture():
class TestSubmarineModelsClientE2E():
def test_model(self, models_client):
- model = LinearNNModel()
+ model = LinearNNModelTorch()
Review comment:
Same problem. Should test all packages.
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model.py
##########
@@ -54,7 +63,7 @@ def test_update_model(self, mocker):
def test_load_model(self, mocker):
mock_method = mocker.patch.object(mlflow.pyfunc, "load_model")
mock_method.return_value = mlflow.pytorch._PyTorchWrapper(
- LinearNNModel())
+ LinearNNModelTorch())
Review comment:
Why only test pytorch?
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model.py
##########
@@ -33,10 +36,16 @@ def tearDown(self):
pass
@pytest.mark.skip(reason="Developing")
Review comment:
This line should be removed, or it will not run this test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu commented on pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu commented on pull request #597:
URL: https://github.com/apache/submarine/pull/597#issuecomment-853078700
Thanks for your contribution.
You currently only test PyTorch model, but the test should cover all package types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu commented on a change in pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu commented on a change in pull request #597:
URL: https://github.com/apache/submarine/pull/597#discussion_r644014035
##########
File path: submarine-sdk/pysubmarine/tests/models/test_model.py
##########
@@ -33,10 +36,16 @@ def tearDown(self):
pass
@pytest.mark.skip(reason="Developing")
Review comment:
this line should be removed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu closed pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu closed pull request #597:
URL: https://github.com/apache/submarine/pull/597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu closed pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu closed pull request #597:
URL: https://github.com/apache/submarine/pull/597
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu commented on pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu commented on pull request #597:
URL: https://github.com/apache/submarine/pull/597#issuecomment-870420696
@featherchen I thought this PR can be closed temporarily because after the discussion of community, we chose to focus only on tf and pytorch distributed training scenes first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] ByronHsu commented on pull request #597: SUBMARINE-822. Expand log_model for more package
Posted by GitBox <gi...@apache.org>.
ByronHsu commented on pull request #597:
URL: https://github.com/apache/submarine/pull/597#issuecomment-870420696
@featherchen I thought this PR can be closed temporarily because after the discussion of community, we chose to focus only on tf and pytorch distributed training scenes first
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@submarine.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org