feat(trainer): Add support for param unpacking in the training function call #62
Conversation
Pull Request Test Coverage Report for Build 17513195086Details
💛 - Coveralls |
|
/lgtm Very nice! |
|
I think it would be great to include this in release 0.1, any thoughts? @briangallagher @andreyvelich @astefanutti |
|
Yes, we should have this before 0.1 to avoid breaking changes. |
|
/milestone v0.1 |
7b748fb to
14c9fdf
Compare
|
@andreyvelich All outstanding comments should be addressed now. |
|
@briangallagher could you rebase the PR please? |
14c9fdf to
47fed69
Compare
0455ee4 to
5304c5d
Compare
|
@andreyvelich I guess the e2e tests will now fail as the func_args are always unpacked. |
There was a problem hiding this comment.
We might need to manually merge this PR, since tests are failing.
@briangallagher Did you verify that your SDK changes are working ?
Yes, tested and working |
0255e84 to
df1a5ea
Compare
|
@briangallagher You might need to rebase this PR once more time |
4f2f25a to
b6764b2
Compare
There was a problem hiding this comment.
Thank you for this great contribution @briangallagher!
/lgtm
/assign @kramaranya @astefanutti @Electronic-Waste
There was a problem hiding this comment.
One small nit, we might need to update type and dostring for func_args here:
sdk/kubeflow/trainer/types/types.py
Line 32 in b6764b2
kubeflow/trainer/utils/utils_test.py
Outdated
| 'apt-get install python-pip\n' | ||
| 'fi\n\n' | ||
| 'PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet ' | ||
| 'PIP_DISABLE_PIP_VERSION_CHECK=1 python -m pip install --quiet ' |
There was a problem hiding this comment.
Why do we need to do that?
There was a problem hiding this comment.
That's require because of how the pip output is formatted here
The reason I have it in this PR is because these tests never actually ran on the previous PR i.e. utils_tests.py was never added to the test-python Make command so I don't think it ever worked.
I've updated this PR now to change the formatting of the "PIP..." output so these spaces are no longer required.
Signed-off-by: Brian Gallagher <briangal@gmail.com>
b6764b2 to
cabe79c
Compare
fixed |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andreyvelich The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thank you @briangallagher! |
What this PR does / why we need it:
Add support for param unpacking in the training function call.
Maintain backward compatibility and add a deprecation warning.
Fixes # 24