FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

BUG: ensure jplephem resources are never leaked at shutdown by neutrinoceros · Pull Request #20238 · astropy/astropy · GitHub

/ astropy Public

BUG: ensure jplephem resources are never leaked at shutdown - #20238

Open
neutrinoceros wants to merge 1 commit into
astropy:mainfrom
neutrinoceros:coordinates/resources-leak
ConversationCommits (1)ChecksFiles changed
Open

BUG: ensure jplephem resources are never leaked at shutdown#20238
neutrinoceros wants to merge 1 commit into
astropy:mainfrom
neutrinoceros:coordinates/resources-leak

Conversation

neutrinoceros commented Aug 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

I noticed this warning was popping up more frequently in my personal runs of the test suite, and while the test already had some guard rails for it, they are clearly not sufficient (and to some extent, misleading, my bad). Let's try this instead.

For completion, here's the warning:

ResourceWarning: Implicitly cleaning up <HTTPError 404: 'Not Found'>
Full trace 
2026-08-15T08:13:50.3223489Z =================================== FAILURES ===================================
2026-08-15T08:13:50.3224291Z ___________________ test_ephemeris_local_file_not_ephemeris ____________________
2026-08-15T08:13:50.3225291Z [gw0] linux -- Python 3.14.7 /home/runner/work/astropy-compatibility-checks/astropy-compatibility-checks/all-nightlies/.venv/bin/python
2026-08-15T08:13:50.3226041Z 
2026-08-15T08:13:50.3226398Z self = <tempfile._TemporaryFileCloser object at 0x7f20e5a1a3c0>
2026-08-15T08:13:50.3226818Z 
2026-08-15T08:13:50.3227044Z     def __del__(self):
2026-08-15T08:13:50.3227424Z         close_called = self.close_called
2026-08-15T08:13:50.3227841Z         self.cleanup()
2026-08-15T08:13:50.3228198Z         if not close_called:
2026-08-15T08:13:50.3228646Z >           _warnings.warn(self.warn_message, ResourceWarning)
2026-08-15T08:13:50.3229593Z E           ResourceWarning: Implicitly cleaning up <HTTPError 404: 'Not Found'>
2026-08-15T08:13:50.3230050Z 
2026-08-15T08:13:50.3230533Z ../../../../../_temp/uv-python-dir/cpython-3.14.7-linux-x86_64-gnu/lib/python3.14/tempfile.py:484: ResourceWarning
2026-08-15T08:13:50.3231113Z 
2026-08-15T08:13:50.3231446Z The above exception was the direct cause of the following exception:
2026-08-15T08:13:50.3231898Z 
2026-08-15T08:13:50.3232143Z cls = <class '_pytest.runner.CallInfo'>
2026-08-15T08:13:50.3232654Z func = <function call_and_report.<locals>.<lambda> at 0x7f20d4f5a560>
2026-08-15T08:13:50.3233182Z when = 'call'
2026-08-15T08:13:50.3233632Z reraise = (<class '_pytest.outcomes.Exit'>, <class 'KeyboardInterrupt'>)
2026-08-15T08:13:50.3234076Z 
2026-08-15T08:13:50.3234294Z     @classmethod
2026-08-15T08:13:50.3234626Z     def from_call(
2026-08-15T08:13:50.3234960Z         cls,
2026-08-15T08:13:50.3235305Z         func: Callable[[], TResult],
2026-08-15T08:13:50.3235762Z         when: Literal["collect", "setup", "call", "teardown"],
2026-08-15T08:13:50.3236376Z         reraise: type[BaseException] | tuple[type[BaseException], ...] | None = None,
2026-08-15T08:13:50.3236962Z     ) -> CallInfo[TResult]:
2026-08-15T08:13:50.3237382Z         """Call func, wrapping the result in a CallInfo.
2026-08-15T08:13:50.3237817Z     
2026-08-15T08:13:50.3238133Z         :param func:
2026-08-15T08:13:50.3238543Z             The function to call. Called without arguments.
2026-08-15T08:13:50.3239180Z         :type func: Callable[[], _pytest.runner.TResult]
2026-08-15T08:13:50.3239629Z         :param when:
2026-08-15T08:13:50.3240017Z             The phase in which the function is called.
2026-08-15T08:13:50.3240454Z         :param reraise:
2026-08-15T08:13:50.3240923Z             Exception or exceptions that shall propagate if raised by the
2026-08-15T08:13:50.3241493Z             function, instead of being wrapped in the CallInfo.
2026-08-15T08:13:50.3241953Z         """
2026-08-15T08:13:50.3242276Z         excinfo = None
2026-08-15T08:13:50.3242636Z         instant = timing.Instant()
2026-08-15T08:13:50.3243011Z         try:
2026-08-15T08:13:50.3243361Z >           result: TResult | None = func()
2026-08-15T08:13:50.3243780Z                                      ^^^^^^
2026-08-15T08:13:50.3244087Z 
2026-08-15T08:13:50.3244398Z ../../.venv/lib/python3.14/site-packages/_pytest/runner.py:361: 
2026-08-15T08:13:50.3244943Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2026-08-15T08:13:50.3245522Z ../../.venv/lib/python3.14/site-packages/_pytest/runner.py:250: in <lambda>
2026-08-15T08:13:50.3246497Z     lambda: runtest_hook(item=item, **kwds),
2026-08-15T08:13:50.3246930Z             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2026-08-15T08:13:50.3247435Z ../../.venv/lib/python3.14/site-packages/pluggy/_hooks.py:512: in __call__
2026-08-15T08:13:50.3248101Z     return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
2026-08-15T08:13:50.3249051Z            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2026-08-15T08:13:50.3249658Z ../../.venv/lib/python3.14/site-packages/pluggy/_manager.py:120: in _hookexec
2026-08-15T08:13:50.3250332Z     return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
2026-08-15T08:13:50.3250893Z            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
2026-08-15T08:13:50.3251497Z ../../.venv/lib/python3.14/site-packages/_pytest/logging.py:865: in pytest_runtest_call
2026-08-15T08:13:50.3252078Z     yield
2026-08-15T08:13:50.3260107Z ../../.venv/lib/python3.14/site-packages/_pytest/capture.py:900: in pytest_runtest_call
2026-08-15T08:13:50.3260956Z     return (yield)
2026-08-15T08:13:50.3261304Z             ^^^^^
2026-08-15T08:13:50.3261855Z ../../.venv/lib/python3.14/site-packages/pluggy/_callers.py:53: in run_old_style_hookwrapper
2026-08-15T08:13:50.3262491Z     return result.get_result()
2026-08-15T08:13:50.3262887Z            ^^^^^^^^^^^^^^^^^^^
2026-08-15T08:13:50.3263432Z ../../.venv/lib/python3.14/site-packages/pluggy/_callers.py:38: in run_old_style_hookwrapper
2026-08-15T08:13:50.3264110Z     res = yield
2026-08-15T08:13:50.3264462Z           ^^^^^
2026-08-15T08:13:50.3264953Z ../../.venv/lib/python3.14/site-packages/_pytest/skipping.py:268: in pytest_runtest_call
2026-08-15T08:13:50.3265528Z     return (yield)
2026-08-15T08:13:50.3265862Z             ^^^^^
2026-08-15T08:13:50.3266414Z ../../.venv/lib/python3.14/site-packages/_pytest/unraisableexception.py:183: in pytest_runtest_call
2026-08-15T08:13:50.3267058Z     collect_unraisable(item.config)
2026-08-15T08:13:50.3267685Z ../../.venv/lib/python3.14/site-packages/_pytest/unraisableexception.py:79: in collect_unraisable
2026-08-15T08:13:50.3268302Z     raise errors[0]
2026-08-15T08:13:50.3268715Z _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
2026-08-15T08:13:50.3269248Z 
2026-08-15T08:13:50.3269546Z config = <_pytest.config.Config object at 0x7f211b8b1a90>
2026-08-15T08:13:50.3269936Z 
2026-08-15T08:13:50.3270211Z     def collect_unraisable(config: Config) -> None:
2026-08-15T08:13:50.3270757Z         pop_unraisable = config.stash[unraisable_exceptions].pop
2026-08-15T08:13:50.3271401Z         errors: list[pytest.PytestUnraisableExceptionWarning | RuntimeError] = []
2026-08-15T08:13:50.3271975Z         meta = None
2026-08-15T08:13:50.3272327Z         hook_error = None
2026-08-15T08:13:50.3272688Z         try:
2026-08-15T08:13:50.3273020Z             while True:
2026-08-15T08:13:50.3273370Z                 try:
2026-08-15T08:13:50.3273731Z                     meta = pop_unraisable()
2026-08-15T08:13:50.3274168Z                 except IndexError:
2026-08-15T08:13:50.3274561Z                     break
2026-08-15T08:13:50.3274911Z     
2026-08-15T08:13:50.3275334Z                 if isinstance(meta, BaseException):
2026-08-15T08:13:50.3275900Z                     hook_error = RuntimeError("Failed to process unraisable exception")
2026-08-15T08:13:50.3276474Z                     hook_error.__cause__ = meta
2026-08-15T08:13:50.3276924Z                     errors.append(hook_error)
2026-08-15T08:13:50.3277341Z                     continue
2026-08-15T08:13:50.3277702Z     
2026-08-15T08:13:50.3278022Z                 msg = meta.msg
2026-08-15T08:13:50.3278387Z                 try:
2026-08-15T08:13:50.3278978Z >                   warnings.warn(pytest.PytestUnraisableExceptionWarning(msg))
2026-08-15T08:13:50.3280022Z E                   pytest.PytestUnraisableExceptionWarning: Exception ignored while calling deallocator <function _TemporaryFileCloser.__del__ at 0x7f211bcfbcc0>: None
2026-08-15T08:13:50.3281080Z 
2026-08-15T08:13:50.3281590Z ../../.venv/lib/python3.14/site-packages/_pytest/unraisableexception.py:67: PytestUnraisableExceptionWarning

Disclaimer: the problem I'm attempting to solve here isn't deterministic and supposedly at least somewhat hard to reproduce since we apparently never hit it in regular CI.

  • 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.

neutrinoceros added this to the v7.2.3 milestone Aug 15, 2026
neutrinoceros added testing Bug backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x labels Aug 15, 2026

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

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.

neutrinoceros force-pushed the coordinates/resources-leak branch from 078b4d4 to 96c8d89 Compare August 15, 2026 09:15
neutrinoceros force-pushed the coordinates/resources-leak branch from 96c8d89 to 4d20f9d Compare August 15, 2026 09:34
neutrinoceros marked this pull request as ready for review August 15, 2026 14:38
mhvk reviewed Aug 16, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This looks OK, except that it would seem we are papering over a bug in jplephem: the _kernel attribute should be deleted on shutdown, and presumably it internally would close its resources. I found the maintainer very responsive, so if you've got a sense of what the problem might be, I suggest raising an issue there (and link to it in the comment here).

neutrinoceros commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

I've contributed to jplephem already. In fact I made it abi3-compliant so it doesn't need a yearly re-release :)
If your assessment that the bug is there is based on the comment I'm removing, I should clarify that I authored it and think, in hindsight, that it is incorrect and misleading.

neutrinoceros commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

To be clear, astropy is really the one managing jplephem.spk.SPK instances, opening them without a defined closing point. And because this happens through public APIs other than context managers, I don't see any other way but to customize the __del__ to ensure that resources are properly finalized.

mhvk reviewed Aug 16, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@neutrinoceros - OK, so the problem is that in _get_kernel() we open the SPK, but we only explicitly close it if we change kernel, and you now add that it gets closed if the science state gets deleted.

That seems reasonable, and I now see in _get_body_barycentric_posvel, we even have an enormous try/except to ensure the SPK gets closed as well.

This then brings up the question if a better solution might be to make _get_kernel a simple class, like,

class _KernelWrapper:
    def __init__(self, value):
        self.kernel = <content of current file>
    def __getattr__(self, attr):
        return getattr(self.kernel, attr)
    def __del__(self):
         <close file>

Or possibly one can just inherit from SPK, like

class _SelfClosingSPK(SPK):
    def __new__(cls, value):
        return cls.open(value)
    def __del__(self):
        self.daf.close()

(but that makes me wonder again whether that would not be a welcome contribution to kplephem).

A scheme like the above would also eliminate the try/except, and allow us to just overwrite _kernel in the science state (which is good since in principle someone might still be hanging on to the kernel, and it is not nice that it gets closed on one if one changes state). What do you think?

mhvk commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Perhaps even simpler,

class SPKAutoCloser(SPK):
    def __del__(self):
        self.daf.close()
        super().__del__()

and then use kernel = SPKAutoCloser.open(value) inside the existing routines.

This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mhvk mhvk left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

backport-v7.2.x on-merge: backport to v7.2.x backport-v8.0.x on-merge: backport to v8.0.x Bug coordinates testing

Projects

None yet

Milestone

v7.2.3

Development

Successfully merging this pull request may close these issues.

2 participants

Back | FazBrowse Home | New Git URL