Context manager for SPICE kernels

[alfonsoSR]

Note
I've read the contributing guidelines and am aware that Kernels' management interfaces are out of your scope. That being said, this could be implemented in less than a minute, and it has improved my personal experience using this library.

Is your feature request related to a problem? Please describe.
I've been working with spicepy for the last year, and one of the only things that annoys me about it is having to use a try-except or try-finally scheme in order to assure that kernels are properly closed, even if an exception is raised. Let me show a litte example:

import spiceypy as spice

kpath = "path/to/metak"

spice.furnsh(kpath)

try:
    t0 = spice.str2et("28-06-2000")
finally:
    spice.kclear()

Describe the solution you'd like
I'd like to be able to use a context manager to automatically load and unload kernels.

with Kernels(kpath):
    t0 = spice.str2et("28-06-2000")

Describe alternatives you've considered
I've been working with a custom, and pretty simple, context manager that looks like this:

class Kernels:

    def __init__(self, path: str) -> None:

        spice.furnsh(path)

    def __enter__(self) -> None:

        return None

    def __exit__(self, exc_type, exc_value, traceback) -> None:

        spice.kclear()

What are your thoughts on this? I'd love to help to implement it if you were open to it.

[AndrewAnnex]

hey @alfonsoSR thanks for writing in. I think in the past I have been asked about adding context managers, specifically #413, that attempted this with larger scope but the author appears to have lost interest in completing that contribution. There may have been too much scope creep in that PR, so maybe you can take that PR and pare it down to the above example?

note the concept of kernel management in the contribution guidelines is intended to avoid adding tools that download kernels for users, so this would be in scope

[alfonsoSR]

@AndrewAnnex I didn't notice there already was an issue regarding this topic. Now that I've read it, I have a couple of questions:

1. I currently assume that kernels are listed in a metakernels file, so that it's path is the only argument the context manager takes. Should I allow to use both the path to a metak file, and a list of paths to individual kernels as arguments?
2. I don't see the point of being able to use the context manager as a decorator for a function, as that makes it more difficult to change the kernels you use.
3. Do you want me to include the _change_dir, and _make_path functions? SPICE already raises a NOSUCHFILE error if any of the required kernels does not exist, and, as you said in Add context manager for a kernel #413, the problem with reading kernels from different directories can be easily solved by adding their paths to the metakernels file.

Just want to know your opinion before starting to work on it.

[AndrewAnnex]

Hey @alfonsoSR, sorry I have been very busy and hadn't had time to think about this, here are my responses:

1. Yes I think so, the context manager should be able to accept variable number of strings/paths. Changing your init function above to __init__(self, *paths: str) and then looping through paths solves that.
2. I'm not sure why using this as a decorator for other functions is desired. I don't remember advocating for that or see where I requested it in the PR. So don't worry about that.
3. I now see the changing directory aspect as a very bad idea now. It could cause a lot of headaches for users not expecting that. Removing those aspects simplifies some things.

Otherwise, I have a few suggestions/questions:

1. The new class maybe should use contextlib.AbstractContextManager base class? possibly unnecessary but could be helpful for static code analysis tools.
2. Maybe use the name KernelPool to keep consistency with conceptually what is happening with Spice/their terminology.
3. Okay the big one, I needed to review the Kernel required reading from the NAIF on this one (https://naif.jpl.nasa.gov/pub/naif/toolkit_docs/C/req/kernel.html). The example you provided doesn't really do what you or other users would want because it isn't side-effect free. Currently, that code would load new kernel(s) then clear the entire kernel pool. Code run after exiting the context manager would not have any kernels loaded into the kernel pool, and any kernels loaded before would still be loaded within the context manager. Luckily, the first problem can be solved by using spice.unload( ) on the kernels included on init instead of using spice.kclear as spice does keep track of individual kernels loaded in a meta kernel. Solving the other problem is harder, but I think solvable by using spice.kdata and spice.kinfo to get a list of the previously loaded kernels to re-load upon exiting the context manager. However there are other gotchas with the kernel pool state that may be unavoidable with this idea so many tests will be needed to ensure the new code does what you want it to do. I recommend reading the required reading for kernels link at the top of this point to see other gotchas with loading/unloading kernels and how that effects the state of the kernel pool. Keep in mind other issues: what if the user changes directory within the context manager? Maybe we can't anticipate everything a user would want to use the context manager for...

Number 3 above gives me a lot of pause on this idea due to the possible complexity involved, but provided enough unit tests demonstrating the functionality I could be convinced... maybe...

[alfonsoSR]

Hi @AndrewAnnex, it's been a while since you answered me, sorry for the delay. I've also been a bit busy this last month.

Using your suggestions as a reference, I have created a pull request with a working version of the context manager. Let me develop a bit on what I have done:

Context manager should be able to accept variable number of strings/paths.

I decided to use a list of strings as input for the context manager. The members of this list may be either paths to individual kernels, paths to meta-kernel files, or a combination of both. Additionaly, the list can be directly passed as an argument for furnsh, so that looping over the paths is not necessary.

Using contextlib.AbstractContextManager as base class

Though I initially intended to implement the context manager as a class with three methods: __init__, __enter__, and __exit__; while I was writing tests I realized about a problem with expceptions raised while loading kernels. It is explained in detail in this StackOverflow question.

Following the suggestion in the accepted answer, I decided to rewrite the context manager using the contextmanager decorator. I guess that's enough to prevent the potential problems with static code analysis (which by the way I haven't experienced.

Using unload instead of kclear

Though I initially thought that using unload allowed for more control, it ended up causing a problem that can be easily fixed by using kclear. Let me describe the problem using an example:

kernel_list = ["kernel_A", "kernel_A", "kernel_B"]

furnsh(kernel_list)

ktotal("all") = 3

# Using unload
unload(kernel_list)
ktotal("all") = 1
kdata = ["kernel_A"]

# Using kclear
kclear()
ktotal = 0

The reason for this unexpected behaviour might have something to do with the following excerpts of documentation:

When a kernel is loaded using furnsh_c, a new entry is created in the database of loaded kernels, whether or not the kernel is already loaded.

The call unload_c ( file ); has the effect of "erasing" the last previous call: furnsh_c ( file );

If furnsh is called twice with the same kernel, it will create two independent entries in the kernel's database. The first time unload is called, it will delete the entry that was created in the last call to furnsh. The second time, it will once again try to delete the last entry, which does not exist anymore. For this reason, I consider that using kclear is a better option.

Make the context manager side-effect free

According to what I have read, the issue with mixing kernels that where loaded before the with statement (global kernels) and those that where loaded by the context manager (local kernels), can be solved using the solution you suggested. I used ktotal, and kdata to create a list with all the global kernels, so that the kernel database can be safely cleared in the context manager initialization, and then restored to its initial state when the context manager is closed.

The only issue with clearing, and then restoring the kernel database is mentioned in the following excerpt of the kernel required reading:

Note that unloading text kernels has the side effect of wiping out any kernel variables and associated values that had been entered in the kernel pool using any of the kernel pool assignment functions, such as pcpool_c. It is important to consider whether this side effect is acceptable when writing code that may unload text kernels or meta-kernels.

I cannot think about a reasonable way to prevent this issue, but loading and unloading kernels in a regular way is likely to be more convenient than using a context manager for this specific case. Explicitly mentioning this behaviour in the documentation is what I would do.

Changing directory inside the context manager

This problem is also mentioned in the kernel required reading:

Changing the working directory from within an application during an application run after calling furnsh_c to load kernels specified using relative paths is likely to invalidate stored paths and prevent open/close and unload operations mentioned above. A simple workaround when this is needed is to specify kernels using absolute paths.

[AndrewAnnex]

hey @alfonsoSR thanks for the detailed response, I've left some comments on the PR that need to be addressed but I'll address some of your response here first.

Agree about worrying about changing directories to be out of scope, users should ensure they pass in absolute paths to kernels in general.

Secondly after looking into it a bit more I agree now with using kclear, although we could detect the type of kernel file, text kernels are so commonly used calling unload on them would end up having the same effect as kclear.

These caveats should be explained in a new docs file and in the docstrings for these functions (see other context manager PR for example...) to make it very clear to users what things to keep in mind.

[alfonsoSR]

Hi @AndrewAnnex , thanks for the feedback! I agree with the need to create documentation for the context manager. In fact, if we want people to know that the context manager exists, it would be great to update current function usage examples to include it. I am also open to work on that, but it is better to do things one at a time.

I don't see any comments in the pull request (leaving the codecov report aside). Have you already left them, or do you intend to do it? There's no rush, I am just asking :).

[AndrewAnnex]

@alfonsoSR sorry forgot to submit the review, mostly formatting but a few things to adjust also

[AndrewAnnex]

@alfonsoSR thanks for the contribution! I've merged your pr in #458, closing this issue.