MAINT: Run PTH flake test in prep for supporting pathlib #16060

MridulS · 2024-02-18T12:20:47Z

This PR starts work on making a unified i/o interface for reading and writing to disk. In some places using strings for filenames work, in other pathlib objects works too. We should be able to pass around Path objects all around the code. For a first go I want to make the PTH ruff flake8 linter happy.

As a start this fixes ruff complaints in couple of sub packages. Didn't want to make the PR too big.

Also want to check if this works with windows CI all fine. (pathlib has had some issues with windows from what I remember).

One way to go about #14893

github-actions · 2024-02-18T12:21:16Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

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 instructions for rebase and squash.
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. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

nstarman

Lots of good things in this PR! I have a few suggestions that are applicable in many places to further simplify the code, e.g. using read_text.

.github/workflows/update_astropy_iers_data_pin.py

astropy/coordinates/name_resolve.py

astropy/coordinates/transformations/graph.py

astropy/timeseries/periodograms/lombscargle/setup_package.py

astropy/visualization/tests/test_lupton_rgb.py

astropy/wcs/setup_package.py

astropy/wcs/tests/test_wcs.py

taldcroft

Looks good for time.

astropy/coordinates/name_resolve.py

astropy/cosmology/_io/tests/test_json.py

astropy/cosmology/tests/test_connect.py

astropy/logger.py

astropy/wcs/tests/test_wcs.py

astropy/wcs/wcsapi/fitswcs.py

astropy/wcs/wcsapi/low_level_api.py

nstarman

Some of the os.path.join got replaced by os-dependent strings.

astropy/modeling/tests/test_polynomial.py

astropy/time/setup_package.py

astropy/wcs/tests/test_pickle.py

astropy/wcs/tests/test_profiling.py

larrybradley

Fine for convolution and stats.

astropy/wcs/setup_package.py

eerovaher · 2024-02-19T19:21:03Z

The way I understand it the intent behind the Ruff PTH rules is that if code is representing filesystem paths as Path instances then it's best to commit to doing that and to avoid converting them to str and then back to Path again. But what seems to be happening in this pull request is that some of the code involving our get_pkg_data_* functions is performing exactly this kind of back and forth shuffling between Path and str that the Ruff PTH rules are meant to guard against. It might therefore be better to ensure that the get_pkg_data_* functions can handle Path instances first, and then it should become possible for the relevant code to use Path throughout.

neutrinoceros · 2024-02-20T05:52:52Z

There might be a slight misunderstanding here, that not what I understand the intention of this rule set to be. Here's the one liner description for flake8-use-pathlib (which ruff reimplements as PTH):

A plugin for flake8 finding use of functions that can be replaced by pathlib module.

That said, I agree with @eerovaher that back-and-forth conversions should be avoided !

pllim

Failure on Windows, see #16060 (comment)

Also please rebase and squash instead of pulling in merge commits, because at its current state, I cannot rebase this PR on top of latest main (gives me conflict). Thanks!

mhvk

Generally, this is a good idea and very nice. That said, quite a few comments. A number are just that if a plain string with no directory is given, open is perfectly fine and reasonable, so no need to replace.

A more general one is whether we can pass "file names" through Path unconditionally; certainly, any time we do, we should adjust the docstring.

Finally, I'd hope we can get rid of a lot of str(path), by ensuring get_package_data_* work - would seem worth doing that here, given how many people have now looked at this...

astropy/coordinates/name_resolve.py

astropy/coordinates/tests/accuracy/generate_spectralcoord_ref.py

astropy/coordinates/transformations/graph.py

astropy/cosmology/_io/tests/test_json.py

astropy/wcs/tests/test_wcs.py

astropy/wcs/wcs.py

docs/conf.py

MridulS · 2024-02-27T10:29:23Z

Finally, I'd hope we can get rid of a lot of str(path), by ensuring get_package_data_* work - would seem worth doing that here, given how many people have now looked at this...

Sure, I am happy to get this into this PR.

Should the changes be backward compatible? I am currently working on making the util functions work with the str and Path objects. Maybe it makes sense to just have a hard break here? (or maybe not as these things are up in the docs https://docs.astropy.org/en/latest/utils/data.html)

If not a hard break, does it make sense to deprecate usage of str only args for future removal?

mhvk · 2024-02-27T13:41:36Z

For this PR at least, I don't think we should have anything that is not backwards compatible. But presumably passing a path through Path is super fast.

astropy/wcs/wcs.py

mcara · 2024-02-27T20:10:10Z

docs/conftest.py

@@ -34,7 +35,7 @@ def _docdir(request):
        # Don't apply this fixture to io.rst.  It reads files and doesn't write.
        # Implementation from https://github.com/pytest-dev/pytest/discussions/10437
        if "io.rst" not in request.node.name:
-            old_cwd = os.getcwd()
+            old_cwd = Path.cwd()


is os.getcwd() getting deprecated?

No, this is to make to standardized usage of PathLike objects rather than str.

(It's fine either way, but to not litter noqa all around we could make this change.

pathlib was introduced in 3.4 and is now pretty feature-complete. Seems a good opportunity to update our approach to paths from the function-oriented manipulation of strings to an object-oriented manipulation of paths. os.getcwd isn't deprecated and probably isn't going to be. It's just the next best thing: superceded.

One thing to keep in mind though is that Path objects are much heavier than strings. It doesn't make a measurable difference here but in intensive loops, switching to pathlib can add a significant overhead.

Yeah, and I don't think there are places where we are doing a lot of string/path manipulation inside astropy (maybe I missed something?). A lot of this is indeed aesthetic-y changes. But IMO we should indeed move on to using the obj oriented pathlib way as it make manipulating these path much easier, plus in a typing heavy future of python, os.PathLike gives a lot more information than str.

astropy/config/tests/test_configs.py

astropy/convolution/setup_package.py

astropy/coordinates/tests/accuracy/generate_ref_ast.py

nstarman · 2024-02-29T04:26:56Z

docs/conftest.py

@@ -34,7 +35,7 @@ def _docdir(request):
        # Don't apply this fixture to io.rst.  It reads files and doesn't write.
        # Implementation from https://github.com/pytest-dev/pytest/discussions/10437
        if "io.rst" not in request.node.name:
-            old_cwd = os.getcwd()
+            old_cwd = Path.cwd()


pathlib was introduced in 3.4 and is now pretty feature-complete. Seems a good opportunity to update our approach to paths from the function-oriented manipulation of strings to an object-oriented manipulation of paths. os.getcwd isn't deprecated and probably isn't going to be. It's just the next best thing: superceded.

nstarman · 2024-04-16T20:44:28Z

@MridulS. This is looking nearly complete. What are the next steps, fixing get_package_data_ first?

MridulS requested review from WilliamJamieson, nstarman, astrofrog, Cadair, mcara, larrybradley and taldcroft as code owners February 18, 2024 12:20

github-actions bot added testing wcs Docs time nddata coordinates stats logging visualization wcs.wcsapi timeseries utils.iers dev-automation labels Feb 18, 2024

MridulS mentioned this pull request Feb 18, 2024

Support pathlib.Path objects wherever possible #14893

Open

nstarman requested changes Feb 18, 2024

View reviewed changes

MridulS requested review from a team as code owners February 19, 2024 09:35

taldcroft approved these changes Feb 19, 2024

View reviewed changes

eerovaher reviewed Feb 19, 2024

View reviewed changes

nstarman requested changes Feb 19, 2024

View reviewed changes

astropy/modeling/tests/test_polynomial.py Outdated Show resolved Hide resolved

astropy/time/setup_package.py Outdated Show resolved Hide resolved

astropy/wcs/tests/test_pickle.py Outdated Show resolved Hide resolved

astropy/wcs/tests/test_profiling.py Outdated Show resolved Hide resolved

larrybradley approved these changes Feb 19, 2024

View reviewed changes

mcara reviewed Feb 19, 2024

View reviewed changes

astropy/wcs/setup_package.py Show resolved Hide resolved

nstarman mentioned this pull request Feb 19, 2024

Use tmp_path in Astropy visualization test #16072

Closed

pllim added this to the v6.1.0 milestone Feb 19, 2024

pllim requested changes Feb 26, 2024

View reviewed changes

MridulS force-pushed the use_tmp_path branch from ead3be5 to 4455c78 Compare February 26, 2024 20:43

mhvk reviewed Feb 26, 2024

View reviewed changes

MridulS requested a review from dhomeier as a code owner February 27, 2024 13:23

mcara reviewed Feb 27, 2024

View reviewed changes

astropy/wcs/wcs.py Outdated Show resolved Hide resolved

mcara reviewed Feb 27, 2024

View reviewed changes

nstarman mentioned this pull request Feb 28, 2024

Address Ruff rule PLR0911 in io #16097

Open

1 task

nstarman reviewed Feb 29, 2024

View reviewed changes

astropy/config/tests/test_configs.py Show resolved Hide resolved

nstarman reviewed Feb 29, 2024

View reviewed changes

MAINT: Run PTH flake test in prep for supporting pathlib

982c78a

MridulS force-pushed the use_tmp_path branch from 375f264 to 982c78a Compare February 29, 2024 08:02

astrofrog modified the milestones: v6.1.0, v7.0.0 Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAINT: Run PTH flake test in prep for supporting pathlib #16060

MAINT: Run PTH flake test in prep for supporting pathlib #16060

MridulS commented Feb 18, 2024 •

edited

github-actions bot commented Feb 18, 2024

nstarman left a comment

taldcroft left a comment

nstarman left a comment

larrybradley left a comment

eerovaher commented Feb 19, 2024

neutrinoceros commented Feb 20, 2024 •

edited

pllim left a comment

mhvk left a comment

MridulS commented Feb 27, 2024

mhvk commented Feb 27, 2024

mcara Feb 27, 2024

MridulS Feb 27, 2024

nstarman Feb 29, 2024

neutrinoceros Feb 29, 2024

MridulS Feb 29, 2024

nstarman Feb 29, 2024

nstarman commented Apr 16, 2024

MAINT: Run PTH flake test in prep for supporting pathlib #16060

Are you sure you want to change the base?

MAINT: Run PTH flake test in prep for supporting pathlib #16060

Conversation

MridulS commented Feb 18, 2024 • edited

github-actions bot commented Feb 18, 2024

nstarman left a comment

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

nstarman left a comment

Choose a reason for hiding this comment

larrybradley left a comment

Choose a reason for hiding this comment

eerovaher commented Feb 19, 2024

neutrinoceros commented Feb 20, 2024 • edited

pllim left a comment

Choose a reason for hiding this comment

mhvk left a comment

Choose a reason for hiding this comment

MridulS commented Feb 27, 2024

mhvk commented Feb 27, 2024

mcara Feb 27, 2024

Choose a reason for hiding this comment

MridulS Feb 27, 2024

Choose a reason for hiding this comment

nstarman Feb 29, 2024

Choose a reason for hiding this comment

neutrinoceros Feb 29, 2024

Choose a reason for hiding this comment

MridulS Feb 29, 2024

Choose a reason for hiding this comment

nstarman Feb 29, 2024

Choose a reason for hiding this comment

nstarman commented Apr 16, 2024

MridulS commented Feb 18, 2024 •

edited

neutrinoceros commented Feb 20, 2024 •

edited