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

Support for YAML 1.2 #8970

Open
maxnoe opened this issue Jul 9, 2019 · 7 comments
Open

Support for YAML 1.2 #8970

maxnoe opened this issue Jul 9, 2019 · 7 comments

Comments

@maxnoe
Copy link
Member

maxnoe commented Jul 9, 2019

As discussed over at gammapy/gammapy#2218 (comment),
@bsipocz asked me to raise this issue here.

the pyyaml library only supports the YAML 1.1 standard from 2005, while only YAML 1.2 added support for scientific notation floating point values.

Summary of differences here:
https://yaml.readthedocs.io/en/latest/pyyaml.html

This is why many packages, e.g. conda itself, switch to ruamel.yaml:
https://yaml.readthedocs.io/en/latest/index.html

@pllim
Copy link
Member

pllim commented Jul 9, 2019

Is anyone from gammapy interested in implementing this here?

@bsipocz
Copy link
Member

bsipocz commented Jul 10, 2019

@taldcroft - would you think we could just swap the usage of pyyaml with ruamel.yaml easily or it would require a more substantial refactor? Or shall we support both?

@maxnoe
Copy link
Member Author

maxnoe commented Jul 10, 2019

Supporting both is not good I think. Because if you have a plain yaml file, it will depend on the library if it is interpreted as 1.1 or 1.2 file.

I found that ruamel.yaml could just be used as a drop in replacement. The usage of the things that got removed from 1.1 to 1.2 is pretty rare I think. You should be able to check if astropy config files even could make use of them.

@bsipocz
Copy link
Member

bsipocz commented Jul 10, 2019

I don't think we can just drop pyyaml and say that from now on we have the other without a deprecation period, but if @taldcroft thinks we can do it, I'm OK with it.

@cdeil
Copy link
Member

cdeil commented Jul 12, 2019

My 2 cents: I'm not sure if switching to ruamel_yaml is a good idea, there's pros and cons that have to be evaluated.

Note that pyyaml is used a lot (https://github.com/yaml/pyyaml/network/dependents) and it looks like development / maintenance was revived in 2016 (https://github.com/yaml/pyyaml/graphs/contributors). The issue with the float parsing mentioned by @maxnoe is yaml/pyyaml#173 and YAML 1.2 support is yaml/pyyaml#116 . I don't know if they plan to implement this, or if it's really important to have.

I don't know who the pyyaml and ruamel_yaml devs are, or if there's a chance that they would collaborate and merge the fork back - having two YAML libs in Python that diverge and having to choose one for each project and argue which to use is really annoying.

The most heavy use of YAML is probably ASDF, so if someone is seriously considering changing Astropy from pyyaml to ruamel_yaml, the first step should probably be to check if they can get the ASDF tests to pass (and I don't know if they have a performance benchmark suite - that might be a concern).

@pllim
Copy link
Member

pllim commented Jul 12, 2019

There is an ASDF PR but it is not merged yet; xref asdf-format/asdf#677

@taldcroft
Copy link
Member

One thing about ruamel_yaml that should be considered is that it appears to be in the midst of a major API change with version 0.15+: https://yaml.readthedocs.io/en/latest/api.html. The docs say that the API is still being fleshed out and production could should stick with <0.15.

That API change might be a good thing in the end, but it seems that right now it might just introduce a lot of churn and trouble for astropy to first transition to the <0.15 API and then we have the mess of possibly supporting <0.15 and 0.15+ ruamel_yaml.

tl;dr - I would propose to wait for ruamel_yaml 1.0 and a stable API before seriously considering a transition for astropy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants