Closed alfonsoSR closed 1 year ago
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
@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:
_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 #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.
Hey @alfonsoSR, sorry I have been very busy and hadn't had time to think about this, here are my responses:
__init__(self, *paths: str)
and then looping through paths solves that. Otherwise, I have a few suggestions/questions:
contextlib.AbstractContextManager
base class? possibly unnecessary but could be helpful for static code analysis tools.KernelPool
to keep consistency with conceptually what is happening with Spice/their terminology. 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...
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:
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.
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.
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.
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.
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.
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.
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 :).
@alfonsoSR sorry forgot to submit the review, mostly formatting but a few things to adjust also
@alfonsoSR thanks for the contribution! I've merged your pr in #458, closing this issue.
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
ortry-finally
scheme in order to assure that kernels are properly closed, even if an exception is raised. Let me show a litte example:Describe the solution you'd like I'd like to be able to use a context manager to automatically load and unload kernels.
Describe alternatives you've considered I've been working with a custom, and pretty simple, context manager that looks like this:
What are your thoughts on this? I'd love to help to implement it if you were open to it.