Allow ALE NaifSpice Mixins to Access SpiceQL service #523

acpaquette · 2023-03-14T04:16:50Z

This update enables the default NaifSpice mixin to access the SpiceQL service, with an example driver updated to test (MroCtxIsisLabelNaifSpiceDriver and MroCtxPds3LabelNaifSpiceDriver).

Both work as expected and can be tested with:
MroCtxIsisLabelNaifSpiceDriver("/Path/to/my/isis/cube.cub", props={"web": True})
or
MroCtxPds3LabelNaifSpiceDriver("/Path/to/my/pds3/label.[lbl|img], props={"web": True})

Keep in mind that so far, this process is much slower over the net and we are still working to improve the speed of the SpiceQL service.

jlaura

I think this needs:

CHANGELOG entries
tests?
thought about the use of use_web all over the place now
tests that handle edge cases introduced by having 3 spice sources or docs describing how this was tested. My concern is unintended breakages due to the new features.

ale/base/data_naif.py

jlaura · 2023-03-23T18:38:34Z

ale/drivers/mro_drivers.py

@@ -344,23 +343,6 @@ def sensor_name(self):
        """
        return "CONTEXT CAMERA"

-    @property


This seems a weird change to include in the PR? CHANGELOG entry might clear up if this is a bug fix for something else or what.

jlaura · 2023-03-23T18:40:16Z

ale/kernel_access.py

+        # to ALE
+        return func(**function_args)
+
+    url = "https://spiceql-dev.prod-asc.chs.usgs.gov/v1/"


Even though this is only an incremental step towards turning this on, I would strongly advocate using an ENV variable here to encode. That seems the most likely way a user might want to set / change this. Sure, this might eventually be hard coded, but I would advocate against that since it tightly couples this ALE version to a specific spiceql server version.

100% agree, I think providing some way to set it then defaulting might be a good middle ground?

jlaura · 2023-03-23T18:41:06Z

ale/kernel_access.py

+        'Content-Type': 'application/json'
+    }
+
+    r = requests.get(url, params=function_args, headers=headers, verify=False)


Yes, this verify=False is needed on a DOI network if the SSL certificates are not in place, but out in the wild this is a bad idea.

Also, requests is not in the meta.yaml so this will fail on install into a clean conda env.

Both good catches!

Revisiting this. Isn't requests is a default python library?

No, it is not. If you want to use just standard library, use urllib (though it does not have the same nice to have stuff like urllib3 or requests).

jlaura · 2023-03-23T18:42:53Z

ale/kernel_access.py

+    r.raise_for_status()
+    response = r.json()
+
+    if r.status_code != 200:


Isn't raise_for_status catching this already? And aren't these redundant with themselves? How does one get a 200 and then a bad status code?

Kinda sorta, this is a weird quirk of our lambda setup that I think needs to change. Basically if the lambda function called fails, we still get a "valid" web return as we catch the failure.

I think we can just let the function itself fail and that should be handled as expected(?)

It may also be that all of our endpoints only handle returning 200 codes.

This likely needs to be ironed out before a full release

ale/kernel_access.py

jlaura · 2023-03-23T18:44:43Z

ale/transformation.py

@@ -123,15 +124,20 @@ def from_spice(cls, sensor_frame, target_frame, center_ephemeris_time, ephemeris

        constant_frames.extend(target_constant_frames)

-        frame_chain.compute_time_dependent_rotiations(sensor_time_dependent_frames, sensor_times)
-        frame_chain.compute_time_dependent_rotiations(target_time_dependent_frames, target_times)
+        # Add all time dependent frame chains to the graph


It seems that use_web is becoming ubiquitous in function signatures. Have other options been considered to not extend the signatures? For example, a module level variable or something?

The function structure is definitely scuffed. Currently the use_web variable is in the NaifSpice mixin, which makes some sense as you want it on a driver to driver level.

Then we have the kernel_access.py module with the spiceql_call wrapper. This wrapper function is then further wrapped in the NaifSpice mixin where the use_web variable is grabbed from the driver when the function is called.

The issue here is that transformation.py and specifically the FrameChain.from_spice code is not part of the driver and exists to support getting the reference frame rotations. This requires that either a variable be passed through or that the FrameChain itself assigns it's own use_web variable which may actually be worth while.

So, to answer your question, I had not considered other options but I don't think a module level define is good. But I think we can make it that any FrameChain constructed using from_spice can set a member variable defining web usage.

ale/transformation.py

acpaquette · 2023-03-24T02:24:51Z

@jlaura This has not been fully tested yet as much of the mocking that happens in the test was not addressed. Basically anything that was mocking a spice call needs to now mock the spiceql_call function. Something that will be addressed in a future sprint where I will go in and basically fix every current NaifSpice driver/tests.

acpaquette · 2023-03-24T02:32:39Z

Also regarding testing the spice sources. I think it's going to be a trust issue, mainly when web is enabled. Pretty much every drivers load test will test passing in kernels manually, and the metakernel discovery function is also tested.

I am not sure if ALE has a responsibility to test if the server returns are accurate. I think we testing the spiceql_call function is necessary but outside of that it's spiceql's problem

Kelvinrr · 2023-07-17T15:31:54Z

ale/spiceql_access.py

+    }
+
+    clean_function_args = stringify_web_args(function_args)
+    async with session.get(url, params=clean_function_args, headers=headers, ssl=False) as response:


ssl=False should probably be optional somehow

ssl=False should be removed. Do we really want to support running without SSL?

(If this is an internal network/custom certificate thing, we can setup our work stations to have the proper self-signed certs.)

you're not wrong, but almost every tool I use on my command line has some kind of disable-ssl option.

Kelvinrr · 2023-07-17T20:48:43Z

ale/spiceql_access.py

+import math
+import requests
+
+import aiohttp


Needs to be added to environment.yml

Kelvinrr

Probably need to wait until. conda-forge/staged-recipes#24039 is merged to merge this.

Kelvinrr · 2023-12-11T21:10:11Z

ale/base/__init__.py

+spiceql_mission_map = {
+    "CHANDRAYAAN-1_M3": "m3",
+    "CHANDRAYAAN-1_MRFFR": "mrffr",
+    "CASSINI_ISS_NAC": "cassini",
+    "CASSINI_ISS_WAC": "cassini",


Should we put this in SpiceQL instead?

It could go in spiceql, the translation has to exist somewhere. I think it's better off in ALE since ALE is using the spiceql interface rather than spiceql conforming to ALE.

Granted all of the keys are from NAIF so 🤷

Kelvinrr · 2023-12-12T04:47:49Z

ale/base/base.py

+                    try:
+                        geodata = gdal.Open(self._file)
+                    except:
+                        geodata = None


Do we not want an exception if the geodata cannot be loaded?

It may be worth reporting the exception to the user as a warning. When working with pds3 labels I was running into issues where the label was pointing to a data file causing the gdal.Open to fail but the label could be used as valid input for a driver.

Kelvinrr · 2023-12-12T05:16:38Z

ale/drivers/selene_drivers.py

+    @property
+    def usgscsm_distortion_model(self):
+        """
+        Kaguya uses a unique radial distortion model so we need to overwrite the
+        method packing the distortion model into the ISD.


Should this be included here? Seems unrelated.

I saw this too and wasn't sure why this was part of the PR. I need to double check this and see if it was missed from a rebase

Update ndarray value type

Kelvinrr · 2024-11-05T16:56:30Z

Closing in favor of #621

acpaquette requested review from Kelvinrr and jlaura March 20, 2023 19:11

jlaura requested changes Mar 23, 2023

View reviewed changes

Kelvinrr reviewed Jul 17, 2023

View reviewed changes

acpaquette force-pushed the spiceql branch from a69963c to 41728f1 Compare September 29, 2023 08:11

acpaquette changed the base branch from spiceql to main September 29, 2023 08:12

jlaura mentioned this pull request Nov 1, 2023

Install just python bindings? DOI-USGS/SpiceQL#7

Closed

acpaquette force-pushed the spiceql branch from 4bb7ac8 to 4d8f269 Compare November 7, 2023 23:23

AustinSanders and others added 20 commits December 11, 2023 10:41

Initial spiceql integration

8aab709

Reverted change to UTC start time

7e26fb5

Fixed erronious spiceql calls

0a89f4b

Moved kernel accessing methods into there own file

0e4c165

Added initial ability to access SpiceQL service

9835056

General drive import and spacing cleanup

ee7a562

Adds spiceql call warpper to kernel access function set

7311697

Line space changes and removed commented line

6a20a5a

Updates tranformation and data_naif with spiceql_calls

187eb7a

Fixes other instances of the from_spice call

a9f28dc

Updates MroCtxIsisLabelNaifSpice Driver to fully use spiceql

f493221

Allow for no kernels to be passed in NaifSpice drivers

e33a638

Addressed mro_ctx_load test

f4d909d

Move write_metakernel_from_cube to kernel_access

f79c918

Added module for access spiceql

ef1aac2

Updated NaifSpice and from_spice to use async functions

d1220aa

Updated mro tests

2d494dc

some minor speed improvements

3e83d34

added some more parallelization

f99a504

Updates to all drivers for spiceql

2cefbaf

acpaquette added 13 commits December 11, 2023 10:57

Updated kaguya drivers and tests

032a994

Updated lro drivers and tests

e8592fd

Updated messenger drivers and tests

b965298

Updated mex drivers and tests

f6f0c61

Updated MGS drivers and tests

a914525

Updated msl drivers and tests

0883096

Updated new horizons tests

ff34212

Updated orex drivers and tests

d07a6e9

Updated themis tests

289620d

Updated viking drivers and tests

8dbcfdb

Fixed up last chandrayaan isd

8e58f89

Moved kernel_access tests to test_kernel_access

8aed755

Minor fixes after rebase

c366767

acpaquette force-pushed the spiceql branch from 26299fe to c366767 Compare December 11, 2023 20:49

acpaquette added 2 commits December 11, 2023 14:02

Added pyspiceql to env file

cd46b88

Change env pyspiceql entry to spiceql entry

6d74087

Kelvinrr reviewed Dec 12, 2023

View reviewed changes

acpaquette force-pushed the spiceql branch from 640cf88 to 6d74087 Compare June 12, 2024 04:14

acpaquette and others added 10 commits June 11, 2024 21:28

Merge spiceql onto dev

8a3f4a3

Fixed missed merge conflict

7946736

Removed util tests that are part of kernel access

bf464ac

Fixed dawn tests

b5f9755

Merge branch 'main' into spiceql

68c93d1

Update ndarray value type

27201fc

Fix typo

209d8ea

Another ndarray fix

a661097

Merge pull request #3 from chkim-usgs/ndarray-fix

6ec7345

Update ndarray value type

Merge branch 'main' into spiceql

021833b

Kelvinrr closed this Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow ALE NaifSpice Mixins to Access SpiceQL service #523

Allow ALE NaifSpice Mixins to Access SpiceQL service #523

acpaquette commented Mar 14, 2023 •

edited

Loading

jlaura left a comment

jlaura Mar 23, 2023

jlaura Mar 23, 2023

acpaquette Mar 24, 2023

jlaura Mar 23, 2023

jlaura Mar 23, 2023

acpaquette Mar 24, 2023

acpaquette May 10, 2023

jlaura Jul 24, 2023

jlaura Mar 23, 2023

acpaquette Mar 24, 2023

jlaura Mar 23, 2023

acpaquette Mar 24, 2023

acpaquette commented Mar 24, 2023

acpaquette commented Mar 24, 2023

Kelvinrr Jul 17, 2023

jlaura Jul 24, 2023

Kelvinrr Jul 24, 2023 •

edited

Loading

Kelvinrr Jul 17, 2023

Kelvinrr left a comment

Kelvinrr Dec 11, 2023

acpaquette Jan 8, 2024

Kelvinrr Dec 12, 2023

acpaquette Jan 8, 2024

Kelvinrr Dec 12, 2023

acpaquette Jan 8, 2024

Kelvinrr commented Nov 5, 2024

Allow ALE NaifSpice Mixins to Access SpiceQL service #523

Allow ALE NaifSpice Mixins to Access SpiceQL service #523

Conversation

acpaquette commented Mar 14, 2023 • edited Loading

jlaura left a 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

acpaquette commented Mar 24, 2023

acpaquette commented Mar 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelvinrr Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelvinrr left a 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

Kelvinrr commented Nov 5, 2024

acpaquette commented Mar 14, 2023 •

edited

Loading

Kelvinrr Jul 24, 2023 •

edited

Loading