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

chore: Fix scientific notation in example yamls #2688

Merged

Conversation

pjanowski
Copy link
Contributor

Description

Example yamls use scientific notation like 1e-5 which yamls parses as str instead of float. Determined is smart enough to convert this to float when submitting experiments to the server but not so when running in local mode. This leads to numerous errors when trying to operate on strings as if they were floats.

Test Plan

Tested running experiments with and without this change locally and on the server/cluster.

Commentary (optional)

Another option would have been to upgrade the _make_local_execution_exp_config method to be smarter about parsing configs for local execution. The change here is simpler and conforms better to intended yaml syntax.

Checklist

  • [x ] User-facing API changes need the "User-facing API Change" label. - not necessary
  • Release notes should be added as a separate file under docs/release-notes/.
    See Release Note for details.
  • [x ] Licenses should be included for new code which was copied and/or modified from any external code. - not necessary

@cla-bot
Copy link

cla-bot bot commented Jul 7, 2021

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @pjanowski on file. In order for us to review and merge your code, please start the CLA process at https://determined.ai/cla.

After we approve your CLA, we will update the contributors list (private) and comment @cla-bot[bot] check to rerun the check.

@netlify
Copy link

netlify bot commented Jul 7, 2021

✔️ Deploy Preview for determined-ui ready!

🔨 Explore the source changes: 156886b

🔍 Inspect the deploy log: https://app.netlify.com/sites/determined-ui/deploys/60e61c31973c770008dbe635

😎 Browse the preview: https://deploy-preview-2688--determined-ui.netlify.app/

@rb-determined-ai rb-determined-ai self-requested a review July 8, 2021 15:13
@rb-determined-ai
Copy link
Member

@pjanowski thank you for bringing this to our attention!

I'm a bit concerned that you are hitting this issue, especially because 1e-5 is definitely a valid float representation according to the yaml 1.2 standard:

[-+]? ( . [0-9]+ | [0-9]+ ( . [0-9]* )? ) ( [eE] [-+]? [0-9]+ )?

(notably, it does not seem to match the regex from the yaml 1.1 standard, replaced by 1.2 in 2009)

So I would say that this is a bug in the yaml parser, not in the config files. I was pretty sure that interanlly we always used ruamel.yaml for parsing yaml, because it handles these cases better than the yaml package.

What are you running to reproduce the error on your local machine?

@pjanowski
Copy link
Contributor Author

What are you running to reproduce the error on your local machine?

Hi Ryan (@rb-determined-ai), thanks for taking a look at this with me. I'm using ruamel.yaml==0.17.10. To confirm, here's a simple script:

import yaml
with open('clm_config.yaml', 'r') as stream:
    d=yaml.safe_load(stream)
lr = d['hyperparameters']['learning_rate']
print(lr)
print(type(lr))

Config: learning_rate: 5e-5
Output

5e-5
<class 'str'>

Config: learning_rate: 5.0e-5
Output

5e-05
<class 'float'>

Does that help?

@rb-determined-ai
Copy link
Member

Yeah, you are using the not-1.2-compatible pyyaml package. There are rumors of 1.2 support with pyyaml, but nothing yet.

Try it with ruamel.yaml:

- import yaml
+ from ruamel import yaml
 with open('clm_config.yaml', 'r') as stream:
     d=yaml.safe_load(stream)
 lr = d['hyperparameters']['learning_rate']
 print(lr)
 print(type(lr))

@pjanowski
Copy link
Contributor Author

Ok, but then neither is Determined (using the ruamel package). I can try to dig into the code, but the behavior I posted above is what you get when using Determined in local mode.

@rb-determined-ai
Copy link
Member

but the behavior I posted above is what you get when using Determined in local mode

Can you share the exact command line you are using to reproduce this behavior? I don't need model code or config files I don't think.

@rb-determined-ai
Copy link
Member

(A stack trace would also be helpful here)

@pjanowski
Copy link
Contributor Author

Yikes, you're absolutely right. Parsing the yaml was on my end. As you suggested using ruamel fixes things. Let me know if you want to me to close this PR or want to update the sample configs (I think the changes would be useful to make it compatible with the standard yaml library which I think most people would use by default.

@rb-determined-ai
Copy link
Member

I'm open to either altering the strings to be pyyaml-compatible, since you already did the work, and I do agree it's a little bit better for users.

Just sign the CLA and we can move forward.

@pjanowski
Copy link
Contributor Author

Just sent it to the specified email.

@rb-determined-ai
Copy link
Member

@cla-bot[bot] check

@cla-bot cla-bot bot added the cla-signed label Jul 13, 2021
@cla-bot
Copy link

cla-bot bot commented Jul 13, 2021

The cla-bot has been summoned, and re-checked this pull request!

Copy link
Member

@rb-determined-ai rb-determined-ai left a comment

Choose a reason for hiding this comment

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

Thank you!

@rb-determined-ai rb-determined-ai merged commit 5a825ad into determined-ai:master Jul 13, 2021
bensomers pushed a commit that referenced this pull request Jul 14, 2021
Example yamls use scientific notation like 1e-5 which pyyaml parses as
str instead of float.  This is because pyyaml, while widespread, is not
a yaml-1.2 compliant parser yet. Improve lives of users a bit by using
yaml 1.1-friendly configs.
@dannysauer dannysauer modified the milestones: 0.0.102, 0.16.3 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants