fix(tracker.py): add base_trial_component_name parameter to create fu…

alex6499cat commented 1 year ago

Issue #, if available:

Description of changes:

@danabens I had to re-create my pull request through my company's Github organization. The changes are the same.

Updated the create function in the Tracker.py file. Previously, this function hardcoded the trial_component_name as "TrialComponent". If a user's IAM role requires them to create the experiment-trial-component resource with a different naming convention, the create fails. I parameterized that as a function parameter named base_trial_component_name and gave it a default of "TrialComponent". This way, the change should not be breaking to anyone.

I also called the new parameter base_trial_component_name because it gets appended with a timestamp. The Sagemaker Estimator also follows the same naming convention with its "base_job_name" parameter https://sagemaker.readthedocs.io/en/stable/api/training/estimators.html.

The unit test test_log_pr_curve in test_tracker.py was failing on Python 3.8 but not other versions of Python. This was because sklearn automatically upgraded to 1.1.1 in python 3.8. I fixed this by forcing all unit tests to use sklearn==0.24.2 because that's the latest version that supports python 36, 37, and 38. I updated tox.ini to do this.

Testing done:

I added an additional assertion on test_create in test_tracker.py to validate the new trial_component_name parameter on Tracker.create.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

[x] I have read the CONTRIBUTING doc
[x] I used the commit message format described in CONTRIBUTING
[x] I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

[x] I have added tests that prove my fix is effective or that my feature works (if appropriate)
[x] I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

alex6499cat commented 1 year ago

Hi @danabens and @owen-t! Will you be able to review my code this week? I'm happy to make any suggested changes.

Thanks!

alex6499cat commented 1 year ago

Hi @danabens. Will you be able to review this pull request? If not, will you tell me who I should contact to have it reviewed?

Thank you,

Alex

danabens commented 1 year ago

./src/smexperiments/tracker.py:189:121: E501 line too long (165 > 120 characters)

alex6499cat commented 1 year ago

./src/smexperiments/tracker.py:189:121: E501 line too long (165 > 120 characters)

Just added a line break to fix that linting issue @danabens. Looks like you have to approve the workflow again.

danabens commented 1 year ago

Run tox -e flake8
flake8 create: /home/runner/work/sagemaker-experiments/sagemaker-experiments/.tox/flake8
flake8 installdeps: flake8, flake8-future-import
flake8 installed: flake8==5.0.4,flake8-future-import==0.4.7,mccabe==0.7.0,pycodestyle==2.9.1,pyflakes==2.5.0
flake8 run-test-pre: PYTHONHASHSEED='321972849[6](https://github.com/aws/sagemaker-experiments/runs/8166119160?check_suite_focus=true#step:5:7)'
flake[8](https://github.com/aws/sagemaker-experiments/runs/8166119160?check_suite_focus=true#step:5:9) run-test: commands[0] | flake8
./src/smexperiments/tracker.py:18[9](https://github.com/aws/sagemaker-experiments/runs/8166119160?check_suite_focus=true#step:5:10):[10](https://github.com/aws/sagemaker-experiments/runs/8166119160?check_suite_focus=true#step:5:11)1: W291 trailing whitespace
ERROR: InvocationError for command /home/runner/work/sagemaker-experiments/sagemaker-experiments/.tox/flake8/bin/flake8 (exited with code 1)
___________________________________ summary ____________________________________
ERROR:   flake8: commands failed

alex6499cat commented 1 year ago

@danabens I fixed that flake8 error and ran all of the linting commands locally to make sure that they worked. My apologies for not doing that sooner. It'll need another workflow approval please.

Thanks!

aws / sagemaker-experiments