Skip to content

make PyYaml a runtime dependency - #11903

Merged
mhvk merged 5 commits into
astropy:mainfrom
nstarman:dependency-pyyaml
Jun 30, 2021
Merged

make PyYaml a runtime dependency#11903
mhvk merged 5 commits into
astropy:mainfrom
nstarman:dependency-pyyaml

Conversation

@nstarman

@nstarman nstarman commented Jun 29, 2021 •
edited by pllim
Loading

Copy link
Copy Markdown
Member

PyYaml >= 3.13

Fixes #11626
Request Review: @taldcroft @mhvk @embray

Signed-off-by: Nathaniel Starkman (@nstarman) [email protected]

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will
review this pull request of some common things to look for. This list
is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label.
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • If the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman
nstarman force-pushed the dependency-pyyaml branch from 1f7d2bd to a492aa1 Compare June 29, 2021 20:30
pllim
pllim reviewed Jun 29, 2021
View reviewed changes
Comment thread astropy/io/misc/hdf5.py Resolved
pllim
pllim reviewed Jun 29, 2021
View reviewed changes
Comment thread astropy/io/misc/tests/test_hdf5.py Resolved
pllim
pllim reviewed Jun 29, 2021
View reviewed changes
Comment thread astropy/io/misc/tests/test_hdf5.py Resolved
pllim
pllim reviewed Jun 29, 2021
View reviewed changes
Comment thread astropy/io/tests/test_registry.py Resolved
pllim
pllim reviewed Jun 29, 2021
View reviewed changes
Comment thread astropy/utils/compat/optional_deps.py Resolved
@pllim pllim added this to the v5.0 milestone Jun 29, 2021
@pllim
pllim requested a review from taldcroft June 29, 2021 20:39
nstarman
nstarman commented Jun 29, 2021
View reviewed changes
Comment thread astropy/io/ascii/ecsv.py Resolved
mhvk
mhvk reviewed Jun 29, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, nice to see this happening - and what a cleanup! Some in-line comments.

A larger one: we happily import numpy and erfa on top of modules, and could do the same with yaml (instead of as needed). I think I prefer the solution here, since yaml is not needed throughout the code unlike numpy and erfa, but I thought I'd bring it up for discussion.

Comment thread astropy/io/fits/connect.py Resolved
Comment thread astropy/io/fits/convenience.py Resolved
Comment thread astropy/io/misc/tests/test_hdf5.py Resolved
Comment thread astropy/io/tests/test_registry.py Resolved
Comment thread astropy/utils/compat/optional_deps.py Resolved
@nstarman
nstarman force-pushed the dependency-pyyaml branch from a492aa1 to 0736bf1 Compare June 29, 2021 21:41
@nstarman

nstarman commented Jun 29, 2021

Copy link
Copy Markdown
Member Author

A larger one: we happily import numpy and erfa on top of modules, and could do the same with yaml (instead of as needed). I think I prefer the solution here, since yaml is not needed throughout the code unlike numpy and erfa, but I thought I'd bring it up for discussion.

There are few places where it might make sense to move import yaml to the top of the module: test_ecsv, io.fits.connect, astropy.table.meta. Anywhere else?

@nstarman nstarman mentioned this pull request Jun 29, 2021
Closed
@nstarman
nstarman requested a review from mhvk June 29, 2021 22:03
@mhvk

mhvk commented Jun 29, 2021

Copy link
Copy Markdown
Contributor

I guess most table/meta/fits use would not involve yaml, so still not sure. Anyway, it is a small dependency (~1MB in ram; though I started with a PET 2001 with 8kB of memory...)

@pllim

pllim commented Jun 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Re: Co-authored-by

It needs name <github email> format, not @user format. You can obtain the correct name <github email> value from git log --author=name. When you put @user in the commit message, the user gets notification spam when the commit gets pushed. FYI.

mhvk
mhvk previously approved these changes Jun 29, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Beyond the one possible leftover import, I think this is all good. Thanks!

@mhvk

mhvk commented Jun 29, 2021

Copy link
Copy Markdown
Contributor

@nstarman - just realized that install.rst also has a section Requirements that needs updating!

@mhvk
mhvk self-requested a review June 29, 2021 22:43
@mhvk
mhvk dismissed their stale review June 29, 2021 22:43

Still one change to be made

@nstarman
make PyYaml a runtime dependency
34ed605 
PyYaml >= 3.13

Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the dependency-pyyaml branch from 0736bf1 to 34e4247 Compare June 29, 2021 23:10
mhvk
mhvk reviewed Jun 29, 2021
View reviewed changes
Comment thread docs/install.rst Resolved
@nstarman @pllim @mhvk
YAML dependency simplifications
51a8adf 
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: Marten van Kerkwijk <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the dependency-pyyaml branch from 34e4247 to f5a5714 Compare June 29, 2021 23:28
mhvk
mhvk approved these changes Jun 29, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great, re-approving!

embray
embray previously requested changes Jun 30, 2021
View reviewed changes

@embray embray left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a few bits of lint to clean up.

Comment thread astropy/coordinates/tests/test_regression.py Resolved
Comment thread astropy/io/ascii/ecsv.py Resolved
Comment thread astropy/io/ascii/tests/test_ecsv.py Resolved
Comment thread astropy/io/fits/connect.py Resolved
Comment thread astropy/io/fits/tests/test_connect.py Resolved
Comment thread astropy/io/misc/hdf5.py Resolved
Comment thread astropy/io/misc/yaml.py Resolved
Comment thread astropy/io/tests/test_registry.py Resolved
Comment thread astropy/io/tests/test_registry.py Resolved
@taldcroft
taldcroft removed their request for review June 30, 2021 13:08
@taldcroft

taldcroft commented Jun 30, 2021

Copy link
Copy Markdown
Member

This looks awesome! Seeing all the extra work we made by having PyYAML be optional, we should have made it a requirement long ago. I took myself off as a reviewer since it is getting a good look from others.

@taldcroft

taldcroft commented Jun 30, 2021

Copy link
Copy Markdown
Member

I'm especially happy now that all the yaml-dependent code will now get a lot more testing in the full build matrix.

@nstarman
nstarman force-pushed the dependency-pyyaml branch from 72ad4de to 88c3af2 Compare June 30, 2021 14:32
nstarman and others added 3 commits June 30, 2021 10:34
@nstarman @pllim @mhvk
Deprecate HAS_YAML
42f6bb3 
Co-authored-by: P. L. Lim <[email protected]>
Co-authored-by: Marten van Kerkwijk <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
move select yaml import to top of module
66547ec 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman @embray
remove extraneous imports
4fcf758 
Co-authored-by: Erik M. Bray <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the dependency-pyyaml branch from 88c3af2 to 4fcf758 Compare June 30, 2021 14:34
@nstarman
nstarman requested a review from embray June 30, 2021 15:08
@mhvk
mhvk dismissed embray’s stale review June 30, 2021 17:42

Comments were addressed

@mhvk

mhvk commented Jun 30, 2021

Copy link
Copy Markdown
Contributor

OK, thanks, @nstarman - with @embray's comments addressed, let's get this in!

@mhvk
mhvk merged commit 4654cde into astropy:main Jun 30, 2021
@nstarman
nstarman deleted the dependency-pyyaml branch June 30, 2021 17:44
eerovaher added a commit to eerovaher/astropy that referenced this pull request Oct 23, 2023
@eerovaher
Remove utils.compat.optional_deps.HAS_YAML
a95d498 
The variable told if `PyYAML` was available, but lost its value when
`PyYAML` was made a required dependency in astropy#11903. It has been
deprecated long enough by now.
@eerovaher eerovaher mentioned this pull request Oct 23, 2023
Merged
1 task done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pllim pllim pllim left review comments
@mhvk mhvk mhvk approved these changes
@embray embray Awaiting requested review from embray

Assignees

No one assigned

Labels

coordinates Docs installation io.ascii io.fits io.misc io.registry table time utils.masked utils

Projects

Cosmology, the Expansion
Status: Done

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

Make PyYAML a strict dependency of astropy

5 participants

@nstarman @mhvk @pllim @taldcroft @embray