Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test infrastructure for external spice kernels #842

Conversation

subagonsouth
Copy link
Contributor

Change Summary

Overview

After discussing with @greglucas, we settled on this quick, partial implementation of what was envisioned as the ultimate solution. These changes allow the use of the the de440s.bsp kernel and can be generalized to include other generic NAIF kernels.

Most of the changes in this PR are adding to testing infrastructure.

  • Ignore all flavors of de440 kernel in git repo to avoid checking in kernel downloaded from NAIF
  • Formally add the pytest marker external_kernel so that tests relying on downloading/using cached kernels can be skipped in most of the github run tests.
  • Add pytest collection hook that automatically adds the _download_de440s fixture to any test marked with external_kernel.
  • Pull out helper functions from the fixture that generates a metakernel from template for reuse
  • Add a function scoped use_test_metakernel fixture that pulls input from metakernel marker to allow specification of different metakernel template files
  • Add a metakernel template for ena simulated data
  • Add test coverage which exercises all of the above

New Files

  • imap_processing/tests/spice/test_data/imap_ena_sim_metakernel.template
    • This will eventually contain all of the kernels needed for processing the simulated ena data

Deleted Files

N/A

Updated Files

  • .gitignore
    • Ignore all flavors of de440 kernel in git repo to avoid checking in kernel downloaded from NAIF
  • imap_processing/tests/conftest.py
    • Add pytest collection hook that automatically adds the _download_de440s fixture to any test marked with external_kernel.
    • Pull out helper functions from the fixture that generates a metakernel from template for reuse
    • Add a function scoped use_test_metakernel fixture that pulls input from metakernel marker to allow specification of different metakernel template files
  • imap_processing/tests/spice/test_geometry.py
    • Add test coverage which exercises all of the above
  • pyproject.toml
    • Formally add the pytest marker external_kernel so that tests relying on downloading/using cached kernels can be skipped in most of the github run tests.

logger.info("Cached kernel file to %s", local_filepath)


def pytest_collection_modifyitems(items):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What else do you envision will go here besides _download_de440s?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible that certain tests could require other large external kernels be downloaded.

r.raise_for_status()
with open(local_filepath, "wb") as f:
for chunk in r.iter_content(chunk_size=8192):
f.write(chunk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger.info("Kernel successfully downloaded: %s", local_filepath)

)
kernel_name = kernel_url.split("/")[-1]
local_filepath = spice_test_data_path / kernel_name

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    if local_filepath.exists():
        logger.info("Kernel file already exists: %s", local_filepath)
        return
    allowed_attempts = 3
    for attempt_number in range(allowed_attempts):

etc.



@pytest.mark.external_kernel()
@pytest.mark.metakernel("imap_ena_sim_metakernel.template")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@@ -72,6 +72,9 @@ filterwarnings = [
"ignore:Converting non-nanosecond:UserWarning:cdflib",
"ignore:datetime.datetime.utcfromtimestamp:DeprecationWarning:cdflib",
]
markers = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious how this will affect codecov checks. For example, if I skip a test codecov complains that there weren't enough tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I don't know how codecov works. It looks like all of the various test runs upload results to the same commit in codecov and so my assumption is that it uses the latest upload. This would mean that the last one to run is the one that gets used in the final check?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They all get combined together actually. So you can skip all but one runner for this. This is also useful for combining Windows-specific, Mac-specific, and Linux-specific behaviors to ensure all your code coverage for those specific platforms gets added together.

@laspsandoval
Copy link
Contributor

Nice work. Looks like some of the test aren't passing. I had a few suggestions, but otherwise looks great. Probably just a couple of minor edits. Ping me and I'll hit the approve button.

Copy link
Collaborator

@greglucas greglucas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach of downloading it somewhere local and .gitignoreing that, good idea!

An interesting approach because I think this will basically force me into downloading it during the tests, whereas I was thinking we'd do a skipif(specific_kernel_not_available) and then have the user run a helper function before the test if they wanted that coverage for example. I kind of like this injection method I think, we'll see once I actually pull it down and test though ;)

Writing this out so I don't forget later, but not relevant for taking action in this PR:

I still have in the back of my mind an idea that I think we might be able to combine all this great kernel management stuff into a singleton class KernelManager, which would have a few methods like load(), download(), write_metakernel(), ... combining basically all of your helper functions here taking care of all loading/unloading of everything. Something like that seems like it might be generically useful. Then in the ensure_spice decorator, we'd check that singleton class for are_kernels_loaded() rather than having the try/except on the spiceypy errors, and could set up some fallbacks to go back to the KernelManager and try to load from the environment variable etc.

imap_processing/tests/conftest.py Outdated Show resolved Hide resolved
subagonsouth and others added 7 commits September 16, 2024 13:28
Ignore all flavors of the de440 SPK
Add external_kernel pytest marker for tests that require kernels to download from NAIF
Parameterize the use_test_metekernel deocorator to enable using alternate metakernel templates
Add metekernel template that includes de440s.bsp
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
@subagonsouth subagonsouth force-pushed the 792-spice-download-and-cache-kernel-files branch from cdb8829 to fa48758 Compare September 16, 2024 19:28
@subagonsouth subagonsouth merged commit 1a363f0 into IMAP-Science-Operations-Center:dev Sep 16, 2024
17 checks passed
@bourque bourque added the SPICE Related to SPICE label Sep 23, 2024
@bourque bourque added this to the Sept 2024 milestone Sep 23, 2024
@subagonsouth subagonsouth deleted the 792-spice-download-and-cache-kernel-files branch September 27, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SPICE Related to SPICE
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants