MNT: Run PTH flake test in prep for supporting pathlib (utils 1/n) (⏰ wait for #16930) by neutrinoceros · Pull Request #16928 · astropy/astropy

neutrinoceros · 2024-09-03T17:51:22Z

Description

Extracted from #16060
~~utils is a big one so I'm focusing on a single module for this PR in order to unblock #16927~~

blocked by

By checking this box, the PR author has requested that maintainers do NOT use the "Squash and Merge" button. Maintainers should respect this when possible; however, the final decision is at the discretion of the maintainer that merges the PR.

github-actions · 2024-09-03T17:51:51Z

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.

github-actions · 2024-09-03T17:51:54Z

👋 Thank you for your draft pull request! Do you know that you can use [ci skip] or [skip ci] in your commit messages to skip running continuous integration tests until you are ready?

pllim · 2024-09-03T17:53:33Z

astropy/utils/data.py

    Parameters
    ----------
-    localfn : str
+    localfn : str | Path


This looks like a new feature? I think this one actually needs a change log.

good point. I need to review this thoroughly myself before I undraft, and I'll keep this in mind when I do !

neutrinoceros · 2024-09-04T06:49:47Z

Actually I find that this only works if the rest of the utils subpackage is updated too. Luckily utils.data is still the bulk of the change. However I also find that this requires a couple small changes in astropy.table to work, so let me open a separate PR for that.

Co-authored-by: Mridul Seth <git@mriduls.com>

pllim

I am a little worried about changing so much low level utility code. Is ruff really that hard to please?

This one might need feedback from downstream like astroquery (@bsipocz) and HPC users (@aarchiba , @weaverba137 , etc).

pllim · 2024-09-04T18:07:15Z

astropy/utils/data.py

    if isinstance(name_or_obj, os.PathLike):
        name_or_obj = os.fspath(name_or_obj)

+    # FIXME: this should be fixed


Can you please elaborate on what exactly should be fixed? Is there a follow-up issue?

pllim · 2024-09-04T18:08:24Z

astropy/utils/data.py

    Parameters
    ----------
-    data_name : str
+    data_name : str | Path


This is considered new feature, so probably needs a change log. Same comment applies to all other similar occurrences in this PR (i.e., adding Path support to public API).

pllim · 2024-09-04T18:11:41Z

astropy/utils/data.py

-                    os.path.join(dldir, entry.name, "url"), encoding="utf-8"
-                )
-                r[url] = os.path.abspath(os.path.join(dldir, entry.name, "contents"))
+    # with os.scandir(dldir) as it:


Why is this commented?

pllim · 2024-09-04T18:12:16Z

astropy/utils/tests/test_data.py

    f2 = download_file(u, cache=True)
-    os.unlink(f2)
-    bf4 = os.path.dirname(f2)
+    # os.unlink(f2)


Why is this commented?

weaverba137 · 2024-09-04T20:31:12Z

I'm not aware of any serious HPC issues with Path objects, but I haven't researched the topic extensively.

neutrinoceros · 2024-09-05T08:43:23Z

This PR isn't ready for review yet.
@pllim I'm having a lot of the same questions you raised, I intend to answer them before I undraft this, but I'll prioritize fixing CI first.

pllim · 2024-10-10T19:29:01Z

There are conflicts now and looks like one of the blockers still unresolved, so moving milestone.

neutrinoceros · 2024-10-31T15:27:38Z

while resolving merge conflicts I realized this is full of unwarranted API changes so I think I'll just drop this PR for now.

neutrinoceros mentioned this pull request Sep 3, 2024

MNT: Run PTH flake test in prep for supporting pathlib #16924

Closed

github-actions bot added the utils label Sep 3, 2024

pllim added this to the v7.0.0 milestone Sep 3, 2024

pllim added no-changelog-entry-needed dev-automation labels Sep 3, 2024

pllim reviewed Sep 3, 2024

View reviewed changes

pllim removed the no-changelog-entry-needed label Sep 3, 2024

pllim mentioned this pull request Sep 3, 2024

MNT: Run PTH flake test in prep for supporting pathlib (modeling) (⏰ wait for #16928) #16927

Closed

1 task

This was referenced Sep 4, 2024

ENH: improve support for Path objects in io.ascii.core.BaseInputter.get_lines #16930

Merged

ENH/BUG: improve support for pathlib.Path inputs in iers #16931

Merged

MNT: Run PTH flake test in prep for supporting pathlib (utils)

f5020da

Co-authored-by: Mridul Seth <git@mriduls.com>

neutrinoceros force-pushed the utils.data/rfc/pth_checks branch from ff2d5d7 to f5020da Compare September 4, 2024 09:05

neutrinoceros changed the title ~~MNT: Run PTH flake test in prep for supporting pathlib (utils.data)~~ MNT: Run PTH flake test in prep for supporting pathlib (utils) Sep 4, 2024

neutrinoceros changed the title ~~MNT: Run PTH flake test in prep for supporting pathlib (utils)~~ MNT: Run PTH flake test in prep for supporting pathlib (utils 1/n) Sep 4, 2024

pllim reviewed Sep 4, 2024

View reviewed changes

neutrinoceros changed the title ~~MNT: Run PTH flake test in prep for supporting pathlib (utils 1/n)~~ MNT: Run PTH flake test in prep for supporting pathlib (utils 1/n) (⏰ wait for #16930) Sep 10, 2024

pllim modified the milestones: v7.0.0, v7.1.0 Oct 10, 2024

neutrinoceros closed this Oct 31, 2024

neutrinoceros deleted the utils.data/rfc/pth_checks branch October 31, 2024 15:27

pllim mentioned this pull request Feb 10, 2025

DOC: Simplify intersphinx resolve astropy when Sphinx minversion is 8.2 #17755

Closed

Uh oh!

Conversation

neutrinoceros commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

github-actions bot commented Sep 3, 2024

Uh oh!

github-actions bot commented Sep 3, 2024

Uh oh!

pllim Sep 3, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

neutrinoceros commented Sep 4, 2024

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

pllim Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

pllim Sep 4, 2024

Choose a reason for hiding this comment

Uh oh!

weaverba137 commented Sep 4, 2024

Uh oh!

neutrinoceros commented Sep 5, 2024

Uh oh!

pllim commented Oct 10, 2024

Uh oh!

neutrinoceros commented Oct 31, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neutrinoceros commented Sep 3, 2024 •

edited

Loading