Add context manager for a kernel #413
Conversation
|
Hello @GEbb4! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-07-04 16:57:13 UTC |
|
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. |
|
@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. |
|
@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: With the contents of Then, in 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: 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
changed the title
|
@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 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 In any case, give this PR a review and let me know what you think :-) |
|
@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: 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. |
|
@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 I'm not certain on next steps etc., do we need input from @jessemapel before proceeding? |
|
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 Report
@@ Coverage Diff @@
## main #413 +/- ##
=======================================
Coverage 99.87% 99.87%
=======================================
Files 12 14 +2
Lines 14731 14817 +86
=======================================
+ Hits 14713 14799 +86
Misses 18 18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
so I think this is looking good, the biggest improvement I think that is needed is the documentation part, but my comments mostly require minor editing/restructuring of the existing file. I also think that the context manager should be called KernelPool to have a tighter conceptual link to how spice manages state, even though I understand the appeal of writing with SpiceKernel...
| @@ -18,5 +18,6 @@ Contents: | |||
| insitu_sensing | |||
| binary_pck | |||
| other_stuff | |||
| extra_features | |||
There was a problem hiding this comment.
this does not belong in the spice lessons, it should be a level up along with exceptions and the cells docs
| as it is implemented in Python there are a small number of extra features to help | ||
| make end-users' code more Pythonic. | ||
|
|
||
| Overview |
There was a problem hiding this comment.
So I actually think it would clearer if this documentation can follow more of a narrative approach that I have in other areas like https://spiceypy.readthedocs.io/en/main/exceptions.html#not-found-errors, and for it to not mirror the spice lessons layout.
currently all the important details are inside the python code block. I think the current examples are fine, just break them apart into separate code blocks, change in in-line comments to paragraphs outside the code blocks, and remove the function def for the first example.
The task statement/ learning goals can then be removed.
|
@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. |
|
@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? |
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:
With this change, this would be reduced to:
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.
However I don't intend to add that for this initial implementation.