728 spice kernel furnishing decorator #734

subagonsouth · 2024-08-06T22:47:59Z

Change Summary

Overview

Sorry for the size of this PR. I don't think that it is possible to break it up any smaller. The most important piece is the ensure_spice decorator. I think that the docstrings are descriptive enough for you all to understand what it does without additional clarification, but let me know if there are any questions. All other changes have to do with setting up testing infrastructure needed to test the decorator.

New Dependencies

None

New Files

imap_processing/tests/spice/test_data/imap_sclk_0000.tsc
- test sclk kernel
imap_processing/tests/spice/test_data/imap_test_metakernel.template
- test metakernel template that is used in the fixture that inserts the local absolute path so that tests work anywhere
imap_processing/tests/spice/test_data/naif0012.tls
- test leapsecond kernel

Deleted Files

None

Updated Files

imap_processing/spice/kernels.py
- Added ensure_spice decorator which will handle automatic kernel furnishing when needed
imap_processing/tests/conftest.py
- Added several testing fixtures used to set up spice kernel needs
- Of particular note is the autouse=true fixture that was added to ensure spice kernels are always cleared after each test
imap_processing/tests/spice/test_kernels.py
- Added tests coverage for ensure_spice decorator with different use cases

Testing

Test coverage for ensure_spice decorator

closes #728

imap_processing/spice/kernels.py

greglucas · 2024-08-07T12:57:24Z

imap_processing/spice/kernels.py

+            except SpiceyError as spicey_err:
+                try:
+                    # Step 2.
+                    metakernel_path = os.environ["SPICE_METAKERNEL"]


Is this standard? Does this mean we are only allowing metakernels and not any other way of loading individual kernels?

If kernels are already furnished, then the Step 1 code will call the function and proceed. This allows for custom/manual furnishing of whatever kernels you want. If they are not furnished, then the secondary default behavior is to look for a metakernel pointed to by this environment variable. We can certainly tweak this functionality as we define details about how we want kernels to be furnished in the pipeline.

It seems like I should put some slides together and set up a meeting to discuss this decorator and the logic flow with the team.

+1 I think an overview of what is happening in ppt would be helpful.

Would we not know if kernels were already furnished? Could we use this

with spice.KernelPool(kernels):

Instead of this?
spice.furnsh(test_sclk) yield test_sclk spice.kclear()

We have also been putting our global configuration / env variables in the imap-data-access repository. I'm not sure if the SPICE configuration options should go over there too.

+1 on an overview. That would be really helpful

greglucas · 2024-08-07T13:00:01Z

imap_processing/tests/conftest.py

+@pytest.fixture(scope="session")
+def monkeypatch_session():
+    from _pytest.monkeypatch import MonkeyPatch
+
+    m = MonkeyPatch()
+    yield m
+    m.undo()
+
+


This isn't being used currently, can we remove it? It looks like you are using the monkeypatch fixture directly below instead of the "session" based one here.
\

greglucas · 2024-08-07T13:15:07Z

imap_processing/tests/conftest.py

+
+
+@pytest.fixture()
+def furnish_sclk(spice_test_data_path):


Looks like this fixture isn't used, do you want to use it here, planning on needing it later? (I assume it might come in future PR of test_time, but just wanted to call it out)

I do think that it will be useful later, especially in test_time. Mostly wanted to put it in to help others with testing fixtures that they might need.

greglucas · 2024-08-07T13:15:10Z

imap_processing/tests/conftest.py

+
+
+@pytest.fixture()
+def use_test_metakernel(tmpdir_factory, monkeypatch, spice_test_data_path):


Do we want to make use of any of the IMAP_DATA directory locations for spice file paths?
https://github.com/IMAP-Science-Operations-Center/imap-data-access/blob/ff6c637533d39a8c5359f2e456e918f2495b0871/imap_data_access/file_validation.py#L354

You might be able to use that location as your tmpdir by accessing imap_data_access.config["DATA_DIR"] / imap/spice/... .

imap_processing/imap_processing/tests/conftest.py

Lines 10 to 15 in b9abcb3

def _set_global_config(monkeypatch, tmp_path):

"""Set the global data directory to a temporary directory."""

monkeypatch.setitem(imap_data_access.config, "DATA_DIR", tmp_path)

monkeypatch.setitem(

imap_data_access.config, "DATA_ACCESS_URL", "https://api.test.com"

)

Hmmm, maybe. I would like to add this to a discussion. I'm not sure I fully grok what you are suggesting but think that it might work better than using this template metakernel.

Sorry, I was not very clear above. I meant to write your template metakernel to that DATA_DIR location (so just pointing to a different tmpdir location, not changing any logic). The DATA_DIR in tests is already set to a temporary directory, so it'd just avoid another temporary directory/location and possibly be closer to where we want the kernels during the mission I think.

greglucas · 2024-08-07T13:17:17Z

imap_processing/tests/spice/test_kernels.py

+    assert func(577365941.184, "ISOC", 3) == "2018-04-18T23:24:31.998"
+
+
+def test_ensure_spice_time_kernels():


Are these next two tests failing because we aren't using the fixture use_test_metakernel? It might be good to add a quick comment about that if so.

subagonsouth · 2024-08-07T15:31:58Z

imap_processing/tests/spice/test_data/imap_sclk_0000.tsc

There is already a clock kernel in the tools directory. I need to compare to this one and keep only one.

subagonsouth · 2024-08-07T15:33:19Z

imap_processing/tests/spice/test_data/naif0012.tls

There is already a leapsecond kernel checked in in the tools directory. We should have only on in the repo. I will either move the other one or point to it in the metakernel template.

laspsandoval

Tenzin is using sds-data-manager/lambda_code/efs_lambda/lambda_function.py to figure out which attitude and ephemeris kernels to use for the symlink. I think getting together to discuss your code and hers and how they work together will be important for this PR.

Another thing that I have been thinking about lately is what about the additional kernels that are not part of the metakernels?

de430.bsp for example I don't think will be delivered to us by the MOC. Something to think about.

It looks good though. A very nice start. Looking forward to discussing. You have boldly gone where none of us dared to go. Nicely done.

laspsandoval · 2024-08-07T16:48:19Z

imap_processing/spice/kernels.py

+            except SpiceyError as spicey_err:
+                try:
+                    # Step 2.
+                    metakernel_path = os.environ["SPICE_METAKERNEL"]


+1 I think an overview of what is happening in ppt would be helpful.

Would we not know if kernels were already furnished? Could we use this

with spice.KernelPool(kernels):

Instead of this?
spice.furnsh(test_sclk) yield test_sclk spice.kclear()

…ice kernels in correct module Add test incomplete test coverage of ensure_spice decorator in correct location

Add leapsecond kernel to test data Add sclk kernel to test data Add metekrnel template to test data

Add test coverage for ensure_spice failure -> SpiceyError

Move metakernel template to other spice kernel location

…aised

greglucas · 2024-08-08T13:21:27Z

imap_processing/spice/kernels.py

+    if f_py and not callable(f_py):
+        raise ValueError(
+            f"Received a non-callable object {f_py} as the f_py argument to"
+            f"ensure_spice.  f_py must be a callable object."
+        )


FYI that my editor says this is unreachable code, so I think it could be removed. i.e. you don't decorate something that isn't callable.

greglucas · 2024-08-08T13:23:40Z

imap_processing/spice/kernels.py

+            except SpiceyError as spicey_err:
+                try:
+                    # Step 2.
+                    metakernel_path = os.environ["SPICE_METAKERNEL"]


We have also been putting our global configuration / env variables in the imap-data-access repository. I'm not sure if the SPICE configuration options should go over there too.

greglucas · 2024-08-08T13:26:47Z

imap_processing/tests/conftest.py

+def spice_test_data_path(imap_tests_path):
+    return imap_module_directory.parent / "tools/tests/test_data/spice"


I think we might want to move these data files into the main package. I'm pretty sure if we released this, the tests would get included in the release, but the test data wouldn't so you wouldn't be able to test the actual sdist because the test data would be missing.

vmartinez-cu · 2024-08-08T15:58:52Z

Nice job on the detailed docstrings!

… test metakernel Move leapsecond and spacecraft clock kernel into imap_processing package

subagonsouth added the SPICE Related to SPICE label Aug 6, 2024

subagonsouth requested review from greglucas, tech3371, vmartinez-cu and laspsandoval August 6, 2024 22:47

subagonsouth self-assigned this Aug 6, 2024

greglucas reviewed Aug 7, 2024

View reviewed changes

subagonsouth commented Aug 7, 2024

View reviewed changes

laspsandoval reviewed Aug 7, 2024

View reviewed changes

subagonsouth added 5 commits August 7, 2024 17:38

Add preliminary decorator that handles automatically furnishing of sp…

6cae621

…ice kernels in correct module Add test incomplete test coverage of ensure_spice decorator in correct location

Add spice kernel furnishing fixtures

136d485

Add leapsecond kernel to test data Add sclk kernel to test data Add metekrnel template to test data

Add test coverage for time kernels only use of ensure_spice

c201889

Add test coverage for ensure_spice failure -> SpiceyError

Remove unused fixture

304956b

Move metakernel template to other spice kernel location

remove duplicate kernel files

78ae704

subagonsouth force-pushed the 728-spice-kernel-furnishing-decorator branch from 217d104 to 78ae704 Compare August 7, 2024 23:38

Add comments about why ensure_spice tests expect an exception to be r…

8ff1136

…aised

greglucas approved these changes Aug 8, 2024

View reviewed changes

Use imap_data_access.config DATA_DIR instead of new tmp_path to write…

5c14546

… test metakernel Move leapsecond and spacecraft clock kernel into imap_processing package

subagonsouth merged commit 4a2642b into IMAP-Science-Operations-Center:dev Aug 16, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

728 spice kernel furnishing decorator #734

728 spice kernel furnishing decorator #734

subagonsouth commented Aug 6, 2024

greglucas Aug 7, 2024

subagonsouth Aug 7, 2024

laspsandoval Aug 7, 2024

greglucas Aug 8, 2024

vmartinez-cu Aug 8, 2024

greglucas Aug 7, 2024

greglucas Aug 7, 2024

subagonsouth Aug 7, 2024

greglucas Aug 7, 2024

subagonsouth Aug 7, 2024

greglucas Aug 8, 2024

greglucas Aug 7, 2024

subagonsouth Aug 7, 2024

subagonsouth Aug 7, 2024

laspsandoval left a comment •

edited

Loading

laspsandoval Aug 7, 2024

greglucas Aug 8, 2024

greglucas Aug 8, 2024

greglucas Aug 8, 2024

vmartinez-cu commented Aug 8, 2024



		@pytest.fixture()
		def use_test_metakernel(tmpdir_factory, monkeypatch, spice_test_data_path):

	def _set_global_config(monkeypatch, tmp_path):
	"""Set the global data directory to a temporary directory."""
	monkeypatch.setitem(imap_data_access.config, "DATA_DIR", tmp_path)
	monkeypatch.setitem(
	imap_data_access.config, "DATA_ACCESS_URL", "https://api.test.com"
	)

		assert func(577365941.184, "ISOC", 3) == "2018-04-18T23:24:31.998"


		def test_ensure_spice_time_kernels():

		def spice_test_data_path(imap_tests_path):
		return imap_module_directory.parent / "tools/tests/test_data/spice"

728 spice kernel furnishing decorator #734

728 spice kernel furnishing decorator #734

Conversation

subagonsouth commented Aug 6, 2024

Change Summary

Overview

New Dependencies

New Files

Deleted Files

Updated Files

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laspsandoval left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmartinez-cu commented Aug 8, 2024

laspsandoval left a comment •

edited

Loading