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

Would it make sense to support the context-manager protocol in skyfield.api.load_file()? #374

Closed
lucabaldini opened this issue May 11, 2020 · 5 comments
Assignees

Comments

@lucabaldini
Copy link

I might be missing something fundamental---in which case I apologize in advance---but I just noticed a resource warning in my unit tests
sys:1: ResourceWarning: unclosed file <_io.BufferedReader name='/data/work/ixpe/ixpeobssim/ixpeobssim/instrument/data/de430t_2000_2040.bsp'> make[1]: Leaving directory '/data/work/ixpe/ixpeobssim/tests'
and I realize that I don't have control (I think) on the files open by skyfield.

Would it make sense to implement the context-manager protocol, so that instead of writing
planets = skyfield.api.load_file(ephem_file_path)
I would do
with skyfield.api.load_file(ephem_file_path) as planets: do something
and have the file closed automatically at the exit of the with block?

@brandon-rhodes
Copy link
Member

The underlying SPK class does have a close() method, that users requested who were having resource issues. What if we added a close() method to the SpiceKernel that calls the underlying close()? Then users could use modern Python’s ability to context-manage it:

from contextlib import closing

with closing(skyfield.api.load_file(ephem_file_path)) as planets:
    ...

Would that work for your use case? I’m hesitant to add too much code to satisfy a mere testing mechanism, in a case where no actual users have yet reported real problems.

@lucabaldini
Copy link
Author

That would be awesome---thanks. And you're right, this is not a real problem by any measure, it's really only a small thing that I noticed testing.

@brandon-rhodes
Copy link
Member

brandon-rhodes commented May 11, 2020

Great! Let's leave the issue open til there's a chance to implement it. In the meantime, I think your tests can call planets.spk.close() for now if you simply want to silence the message — or can even do:

from contextlib import closing

planets = skyfield.api.load_file(ephem_file_path)
with closing(planets.spk):
    ...

@lucabaldini
Copy link
Author

Now that I think about it, I'd say that, had I found the snippet above in the docs, I wouldn't have opened this issue in the first place. (And I apologize in advance if it's there already and I missed it.)

So maybe adding some direction in the docs is really everything that one needs?

@brandon-rhodes brandon-rhodes self-assigned this May 13, 2020
@brandon-rhodes
Copy link
Member

It should certainly be mentioned in the docs, yes, and no you didn’t miss it — no one had ever asked, and I had not thought of putting it there myself.

When I get around to adding the docs, I'll see if I also feel inspired to make it more convenient than it currently is :)

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

No branches or pull requests

2 participants