docs: update docs for YAML 1.2 compatibility mitigation #3579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dberenbaum
reviewed
May 23, 2022
dberenbaum
reviewed
May 23, 2022
| @@ -58,6 +58,10 @@ as the tree path to find those values. Supported types are: string, integer, | |||
| float, and arrays (groups of params). Note that DVC does not ascribe any | |||
| specific meaning to these values. | |||
|
|
|||
| > YAML 1.2 stores very large and very small numbers in scientific notation. Some | |||
There was a problem hiding this comment.
I like that you specify a common problem here with scientific notation. In the note below, I like that you specify that PyYAML doesn't support YAML 1.2. Maybe you can combine them and have pretty much the same note in both places?
It would also be great to suggest solutions in both places (ruamel.yaml, JSON or other file formats).
|
Thank you @bobertlo! I left some comments but also open to merging as is and iterating since it's already an improvement. |
|
Sure! I had similar thoughts about the structure of the changes and wasn’t
sure how to approach it.
How would you feel about making “parameter values” a (linkable) heading
with a full description and in a note offering solutions and the code
examples could just mention PyYAML briefly after the examples and link back?
I could send a revision back for review with that later and I will look for
the other examples using PyYAML at least to include them in this review.
…On Mon, May 23, 2022 at 11:02 AM Dave Berenbaum ***@***.***> wrote:
Thank you @bobertlo <https://github.com/bobertlo>! I left some comments
but also open to merging as is and iterating since it's already an
improvement.
—
Reply to this email directly, view it on GitHub
<#3579 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGWHGMAIMGPHOOWABEG7ELVLOT2BANCNFSM5WR2DC6A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@bobertlo I'm not sure without seeing it but could be nice. As I mentioned before, I'm open to merging as is or in any form that's better than what we have today (no warning about YAML incompatibility). |
|
Maybe it would be good to just merge this and the other points can be
addressed separately.
My first concern is catching the “dvc parameters scientific notation”
Google search with a solution ;)
…On Mon, May 23, 2022 at 1:29 PM Dave Berenbaum ***@***.***> wrote:
@bobertlo <https://github.com/bobertlo> I'm not sure without seeing it
but could be nice. As I mentioned before, I'm open to merging as is or in
any form that's better than what we have today (no warning about YAML
incompatibility).
—
Reply to this email directly, view it on GitHub
<#3579 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGWHGKSVJJJ4WOXTKDL4QLVLPFADANCNFSM5WR2DC6A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
dberenbaum
approved these changes
May 23, 2022
See iterative/dvc#5971 use ruamel.yaml in examples instead of PyYAML and add warnings add note to parameter values section metioning scientific notation for SEO
|
I did my best to reconcile some of the ideas mentioned and got the other two places where Feel free to make any language edits you feel are helpful if needed. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See iterative/dvc#5971
adds note to parameter values section metioning scientific notation for SEO
use ruamel.yaml in example instead of PyYAML and add note
❗ Please read the guidelines in the Contributing to the Documentation list if you make any substantial changes to the documentation or JS engine.
🐛 Please make sure to mention
Fix #issue(if applicable) in the description of the PR. This causes GitHub to close it automatically when the PR is merged.Please choose to allow us to edit your branch when creating the PR.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏