Add context manager for a kernel

[GeorgeEbberson]

Still WIP, opening to get opinions before I sink time into testing/docs.

Adds a context manager (which can be used as a decorator) for running some code with given kernels loaded. This cuts down on boilerplate and makes the resulting user code more pythonic. The original motivation behind this is because I was using this package for a research project and ended up solving this problem myself organically, then thought the community find use for it too!

Imagine a project who stores the kernels in a different location to the code, and who runs in some third location. An example snippet might be:

from os import chdir, dirname, getcwd
import spiceypy as spice

METAKERNEL = r"/path/to/metakernel.txt"

start_dir = getcwd()
chdir(dirname(METAKERNEL))
spice.furnsh(METAKERNEL)
pos, _ = spice.spkpos(name, times, frame, correction, reference)
spice.unload(METAKERNEL)
chdir(start_dir)

With this change, this would be reduced to:

import spiceypy as spice
from spiceypy.context import SpiceKernel

METAKERNEL = r"/path/to/metakernel.txt"

with SpiceKernel(METAKERNEL):
    pos, _ = spice.spkpos(name, times, frame, correction, reference)

On top of the more pythonic code, there are extra advantages - if an error occurs in the first snippet, it must be handled and the kernels manually unloaded, but because python context managers' exit methods run regardless, the second option should always unload the kernels.

I've intentionally put the context manager in a separate namespace to all the SPICE-mirroring function names, to make clear that this is something extra. It's possible for future updates that the context manager object itself could be caught to do things to the current context, e.g.

with SpiceKernel(METAKERNEL) as krnl:
    pos, _ = spice.spkpos(name, times, frame, correction, reference)
    krnl.add(r"/path/to/extra/kernel.txt")
    # Do something with the extra data here.

However I don't intend to add that for this initial implementation.

[pep8speaks]

Hello @GEbb4! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

• In the file spiceypy/context.py: No issues found.

• In the file spiceypy/spiceypy.py: No issues found.

• In the file spiceypy/tests/test_context.py: No issues found.

Comment last updated at 2021-07-04 16:57:13 UTC

[jessemapel]

This looks very handy and you can even pass lists of kernels as furnsh and unload can take them. I'm confused why you are changing directory around the kernel loading and unloading. SPICE doesn't require you be next to your kernels while using them.

[AndrewAnnex]

@GEbb4 thanks for the contribution and I look forward to seeing how this progresses. @jessemapel has a good point about changing directories relative to the metakernel as spice meta kernels can support both relative and absolute paths, (https://naif.jpl.nasa.gov/pub/naif/toolkit_docs/C/req/kernel.html#Path%20Symbols%20in%20Meta-kernels) so it may not make sense to always change the current working directory, although I forget if spice knows to resolve the absolute paths relative to the meta-kernel on it's own. If spice doesn't resolve the relative paths it could make sense to have two context managers, one with the current functionality, to allow this. As per the contribution guidelines this pr will need tests before it could be merged, and documentation additions in the form of a new short tutorial would also be beneficial.

This change and possibly #410 also starts to broach the issue of what is "in-scope" for spiceypy and what is not. Originally I always envisioned spiceypy as a spice wrapper and nothing else, anything that attempts to expand the functionality beyond the spice api was out of scope. However, I have also broken this policy a bit by the addition of a few simple convenience utilities from users that were sufficiently small. I think that this pr, likely without the directory changing bit, is sufficiently self-contained and simple that it is fine to include in spiceypy, and is more inline with this policy than other changes in fact, but for future contributions I may need to create a spiceypy-contrib project to capture higher level utilities that use spiceypy to give those changes a home.

[GeorgeEbberson]

@jessemapel @AndrewAnnex thanks both for your replies, since you both seem happy I'll continue developing and go ahead with testing, docs, etc., although I have a hectic few months ahead so progress might be a little slow.

@jessemapel the directory changing is because spice doesn't resolve files in the same directory as the metakernel if the metakernel has been given absolutely. Here's a simplified version of the original example which made me write it like that in the first place:

.
+-- spice_files
|   +-- combined.bsp
|   +-- gm_de431.tpc
|   +-- metakernel.txt
|   +-- naif_0012.tls
|   +-- pck00010.tpc
+-- spice_demo.py

With the contents of metakernel.txt being:

\begindata
KERNELS_TO_LOAD=(
'combined.bsp',
'gm_de431.tpc',
'naif0012.tls',
'pck00010.tpc')
\begintext

Then, in spice_demo.py:

import spiceypy as spice

METAKERNEL = r"D:\dev\solarsail\src\solarsail\spice_files\metakernel.txt"

spice.furnsh(METAKERNEL)
pos = spice.str2et("2020-07-10 11:00 AM")
spice.unload(METAKERNEL)

print(pos)

Which, if the code is run in any other directory, leads to an error:

Traceback (most recent call last):
  File "spice_demo.py", line 8, in <module>
    spice.furnsh(METAKERNEL)
  File "D:\virtualenvs\sail\lib\site-packages\spiceypy\spiceypy.py", line 108, in with_errcheck
    check_for_spice_error(f)
  File "D:\virtualenvs\sail\lib\site-packages\spiceypy\spiceypy.py", line 91, in check_for_spice_error
    raise stypes.dynamically_instantiate_spiceyerror(
spiceypy.utils.exceptions.SpiceNOSUCHFILE:
================================================================================

Toolkit version: CSPICE66

SPICE(NOSUCHFILE) --

The first file 'combined.bsp' specified by KERNELS_TO_LOAD in the file D:\dev\solarsail\src\solarsail\spice_files\metakernel.txt could not be located.

furnsh_c --> FURNSH --> ZZLDKER

================================================================================

By changing directory in the context manager, this error will never be thrown and the directory change should be transparent to the user as it only happens when kernels are loaded/unloaded.

@AndrewAnnex you talk of having two separate context managers, one with and one without this functionality. I wonder if just having a kwarg switch for it would be preferable so that there's only ever one object, and making switching directory the default as it gives the least chance of a user seeing an error? Although I'm not sure that there's ever a situation where we don't want to do the directory change, so I'm not sure if there's much merit in having that at all...?

[GeorgeEbberson]

@AndrewAnnex I think this is ready for review/merging now. I've added a kwarg which lets the user entirely disable directory changing if they so wish, as well as a handful of tests.

Your comments about what are in and out of scope have also gotten me thinking. IMO there's quite a big market for higher-level abstractions in spiceypy, because I'd say anyone using Python is happy to take more pythonic code at the expense of complexity etc. - otherwise, if it was really important, they'd likely learn/use C.

For example, another of the things I've found really difficult to do is check for kernels being loaded. You could write a relatively simple utility class which checks all of the currently loaded kernels, which would allow you to do:

if "gm_de431.tpc" in LoadedKernels:

Which is extremely pythonic, but would that be in scope? Probably not, based on the current scope statement. And IMO, you'd be better off having a spiceypy-core library which is just the essentials, and keeping spiceypy containing everything (with the core being a dependency) - because most users will want the extra, more pythonic features. Or, honestly, just keeping everything together - does it really need to be separated? Maybe a new issue to capture full discussion of it would be worthwhile?

In any case, give this PR a review and let me know what you think :-)

[AndrewAnnex]

@GEbb4 thanks for the changes although @jessemapel 's point still stands regarding changing the directories with meta kernels.

Given your example path and meta kernel, SPICE supports specifying the relative paths using the following path symbol syntax:

\begindata

PATH_VALUES = ( 'D:\dev\solarsail\src\solarsail\spice_files', )

PATH_SYMBOLS = ( 'DIR', )

KERNELS_TO_LOAD=(
'$DIR\combined.bsp',
'$DIR\gm_de431.tpc',
'$DIR\naif0012.tls',
'$DIR\pck00010.tpc')
\begintext

That said, I now think changing directory by default mostly seems safe with respect to spice and spice kernels using the above syntax. If a kernel uses that syntax to define relative or absolute paths, changing directory to that kernel should make no difference. And since the context manager is using a context manger for changing directories it seems to be side-effect free with respect to functions using it as the cwd will return to what is expected at the end of the enter function, at least I believe it will.

[GeorgeEbberson]

@AndrewAnnex yes, the user absolutely should not notice that there's a directory change happening, but it does reduce the number of error cases. Whilst the kernel you show does indeed mitigate the problem, if there's a possible error case that we can solve here I see no reason not to, hence the implementation I've made.

And if there really is a problem being caused, say some security case which is sensitive to which directories are accessed or some obscure set of permissions, then the user can add the allow_change_dir=False flag and the directory changing is entirely disabled. But, since most users won't care about this, it's enabled by default to stop throwing the error case described above.

I'm not certain on next steps etc., do we need input from @jessemapel before proceeding?

[AndrewAnnex]

No there is no formal review process really unless I have specified it in the contributing docs. I just need to think it over a bit more and do a thorough review of the actual code/docs and I’ll need to enable the CI builds to ensure the coverage doesn’t decrease significantly and that the tests pass. Other opinions of course are welcome but I think overall we have worked through most/all of my concerns. Just give me a few days to find some time to do the above and look out for a code review from me.

-Andrew Annex

On Jul 4, 2021, at 5:05 PM, GEbb4 @.***> wrote:

 @AndrewAnnex yes, the user absolutely should not notice that there's a directory change happening, but it does reduce the number of error cases. Whilst the kernel you show does indeed mitigate the problem, if there's a possible error case that we can solve here I see no reason not to, hence the implementation I've made.

And if there really is a problem being caused, say some security case which is sensitive to which directories are accessed or some obscure set of permissions, then the user can add the allow_change_dir=False flag and the directory changing is entirely disabled. But, since most users won't care about this, it's enabled by default to stop throwing the error case described above.

I'm not certain on next steps etc., do we need input from @jessemapel before proceeding?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[codecov[bot]]

Codecov Report

Merging #413 (e21342e) into main (8232fc9) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #413   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          12       14    +2     
  Lines       14731    14817   +86     
=======================================
+ Hits        14713    14799   +86     
  Misses         18       18           

Impacted Files	Coverage Δ
spiceypy/spiceypy.py	99.69% <ø> (ø)
spiceypy/context.py	100.00% <100.00%> (ø)
spiceypy/tests/test_context.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8232fc9...e21342e. Read the comment docs.

[AndrewAnnex]

@GEbb4 I want to thank you for the effort in this PR. As per the discussion in #417, I think that this contribution is in scope, but there are some code changes/docs that are needed to the PR to get it merged, I won't have the time to do the changes myself for a few months. I'm closing the PR for now but it may be able to reopened at some point or moved to a contrib package for spiceypy.

[GeorgeEbberson]

@AndrewAnnex I realise I just sort of disappeared on this; but I've restarted working on the project which lead to me bringing it up in the first place and now intend to finish it off - if you're still happy for me to go ahead? I can't see any evidence of a second package for abstractions so I presume it's still in-scope?