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

01_getting_started_pytorch notebook - Hyperparameters sent by the client aren't passed to the Training Arguments #67

Closed
hellosamstuart opened this issue Aug 4, 2021 · 2 comments · Fixed by #68

Comments

@hellosamstuart
Copy link

Hello!

Reopening an issue connected to this thread here:

Thank you HuggingFace team for all you do! This summer I have been working from this notebook when I noticed a gap I will discuss below. PS - this is my first GitHub issue, if you have any feedback.

Description (same as 52)

The hyperparameters sent by the client have an underscore in them (e.g. output_data_dir), whereas those received by the argparser have a hyphen (e.g. output-data-dir). Therefore, values do not get propagated through the train.py file.

Why another issue?

There have been recent commits resolving the above-linked issue in most of the notebooks. However I noticed the commit to fix this issue for Pytorch missed the second half of the typos (commit fixed train-batch-size and eval-batch-size, but still need to fix output-data-dir and model-dir).

Relevant Commits

Files

I have tested the solution on these files

notebooks/sagemaker/01_getting_started_pytorch/sagemaker-notebook.ipynb
notebooks/sagemaker/01_getting_started_pytorch/scripts/train.py

Solution

In the train.py file, swap these lines -

parser.add_argument("--output-data-dir", type=str, default=os.environ["SM_OUTPUT_DATA_DIR"])
parser.add_argument("--model-dir", type=str, default=os.environ["SM_MODEL_DIR"])
with these

parser.add_argument("--output_data_dir", type=str, default=os.environ["SM_OUTPUT_DATA_DIR"])
parser.add_argument("--model_dir", type=str, default=os.environ["SM_MODEL_DIR"])

@hellosamstuart
Copy link
Author

Hi @philschmid! Got a tip from my colleague who filed the issue #52 that tagging you here would be best for Sagemaker issues. Please see my note above about a gap a recent commit leaving a remaining issue behind. Thanks!

@philschmid philschmid mentioned this issue Aug 4, 2021
@philschmid
Copy link
Member

Hey @hellosamstuart,
Thank you for finding those! Should be merged and good to use.

You must have spent way too much time creating this issue. It looks so well structured. BTW you can always open a PR with the fix too

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 a pull request may close this issue.

2 participants