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.

Sign up for GitHub

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

Jump to bottom

DM-41941: Handle all options for RemoteButler.get() inputs #923

Merged

dhirving merged 8 commits into main from tickets/DM-41941

Dec 14, 2023

Contributor

dhirving commented Dec 8, 2023

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

codecov bot commented Dec 8, 2023 •

edited

Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (40b9058) 87.60% compared to head (166fc17) 87.63%.

Files	Patch %	Lines
...t/daf/butler/remote_butler/server/_dependencies.py	33.33%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #923      +/-   ##
==========================================
+ Coverage   87.60%   87.63%   +0.03%     
==========================================
  Files         295      295              
  Lines       38254    38327      +73     
  Branches     8088     8097       +9     
==========================================
+ Hits        33511    33588      +77     
+ Misses       3539     3535       -4     
  Partials     1204     1204

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dhirving force-pushed the tickets/DM-41941 branch 2 times, most recently from ee6a532 to 61b7dee Compare

December 11, 2023 17:03


          Implement RemoteButler.get() via DataId

8f89ffd

dhirving force-pushed the tickets/DM-41941 branch 5 times, most recently from 96f5018 to c54ad78 Compare

December 13, 2023 00:28

dhirving marked this pull request as ready for review

December 13, 2023 00:30

timj approved these changes

View reviewed changes

Member

timj left a comment

Looks great. Nice cleanups.

python/lsst/daf/butler/dimensions/_coordinate.py

@@ @@ -69,17 +76,20 @@ @@
               DataCoordinate or other data ID.
               """
+              SerializedDataId = dict[str, DataIdValue]

Member

timj Dec 13, 2023

Oddly, DataId is defined below to use Any instead of DataIdValue.

Contributor Author

dhirving Dec 13, 2023

You mean in this bit?:

DataId = DataCoordinate | Mapping[str, Any]

Not really sure what that's about. DataCoordinate proper does seem to use the DataIdValue pretty consistently everywhere. That DataId union exists mainly as a parameter for Butler methods I think, so maybe when you guys added mypy there was too much random code that was passing in like stringifiable things that weren't actually strings or something. There is a bit of code in standardize doing things like coercing numpy.int64 to int, but not as much as I would have thought.

I kind of feel like there are a values that aren't ints or strings though. Isn't some stuff like floats or datetimes?

I sort of suspect that the existing QuantumGraph code or wherever this is getting used just happens to not use anything unusual, and we may end up with some stuff getting accidentally coerced to strings at some point. In any case I just pulled out an alias for the existing definition from SerializedDataCoordinate, so a problem for another day.

Member

timj Dec 14, 2023

Dimensions do not use floats because you can't do equality on them. Dimensions are strings or int (and as you found, integral types so really should be numbers.Integral or something instead of int).

python/lsst/daf/butler/dimensions/_coordinate.py

-                  def direct(
-                      cls, *, dataId: dict[str, DataIdValue], records: dict[str, dict] | None
-                  ) -> SerializedDataCoordinate:
+                  def direct(cls, *, dataId: SerializedDataId, records: dict[str, dict] | None) -> SerializedDataCoordinate:

Member

timj Dec 13, 2023

Maybe change the type of the dataId parameter in the docstring below (although last I looked Sphinx can get upset with type aliases).

python/lsst/daf/butler/remote_butler/_remote_butler.py Outdated

-                  def _simplify_dataId(
-                      self, dataId: DataId | None, **kwargs: dict[str, int | str]
-                  ) -> SerializedDataCoordinate | None:
+                  def _simplify_dataId(self, dataId: DataId | None, kwargs: dict[str, int | str]) -> SerializedDataId:

Member

timj Dec 13, 2023

Suggested change

      
                def _simplify_dataId(self, dataId: DataId | None, kwargs: dict[str, int | str]) -> SerializedDataId:
          
                def _simplify_dataId(self, dataId: DataId | None, kwargs: dict[str, DataIdValue]) -> SerializedDataId:

?

python/lsst/daf/butler/remote_butler/_remote_butler.py Outdated

                       Returns
                       -------
-                      data_id : `SerializedDataCoordinate` or `None`
+                      data_id : `SerializedDataId` or `None`

Member

timj Dec 13, 2023

Suggested change

      
                    data_id : `SerializedDataId` or `None`
          
                    data_id : `SerializedDataId`

python/lsst/daf/butler/remote_butler/_remote_butler.py Outdated

+                          dataset_id = datasetRefOrType.id
+                          response = self._get(f"get_file/{dataset_id}")
+                          if response.status_code == 404:
+                              raise LookupError(f"Dataset not found with ID {dataset_id}")

Member

timj Dec 13, 2023

I know this was in the old code but it might be better to print the datasetRefOrType here and not just the ID since that is more human readable (and will include the ID).

python/lsst/daf/butler/remote_butler/_remote_butler.py

+                      ref = DatasetRef.from_simple(
+                          self._parse_model(response, SerializedDatasetRef), universe=self.dimensions
+                      )
+                      if storage_class is not None:

Member

timj Dec 13, 2023

Thanks for fixing this.

python/lsst/daf/butler/remote_butler/_remote_butler.py

+                      """Convert DatasetType parameters in the format used by Butler methods
+                      to a standardized string name for the REST API.
+                      """
+                      if isinstance(datasetTypeOrName, DatasetType):

Member

timj Dec 13, 2023

Would

return getattr(datasetTypeOrName, "name", datasetTypeOrName)

be more efficient?

Contributor Author

dhirving Dec 14, 2023

Slightly, yeah, but less type safe and more ambiguous. I personally have no idea off the top of my head whether str or any str-like objects that may be passed in by a user have a name attribute or not.

python/lsst/daf/butler/remote_butler/server/handlers/_external.py

+                      )
+                      return _get_file_by_ref(butler, ref)
+                  except LookupError as e:
+                      raise NotFoundException() from e

Member

timj Dec 13, 2023

No error message?

Contributor Author

dhirving Dec 13, 2023

It's not actually shipping the error message from those NotFoundExceptions to the client yet, and the from e will put the original error message in the server logs. I suppose I could have put in "DataId not found" or something.

In a couple weeks when I go in and set up the error propagation framework, that try block will go away entirely.

tests/test_server.py Outdated

+                      def check_sc_override(converted):
+                          self.assertNotEqual(type(metric), type(converted))
+                          self.assertIs(type(converted), new_sc.pytype)

Member

timj Dec 13, 2023

Suggested change

      
                        self.assertIs(type(converted), new_sc.pytype)
          
                        self.assertIsInstance(converted, new_sc.pytype)

?

python/lsst/daf/butler/datastores/fileDatastoreClient.py

@@ @@ -81,6 +84,8 @@ def get_dataset_as_python_object( @@
                   ]
                   ref = DatasetRef.from_simple(payload.dataset_ref, universe=universe)
+                  if component is not None:

Member

timj Dec 13, 2023

Maybe note that you have to apply the component override before the storage class override.

dhirving added 7 commits

December 14, 2023 10:55


          Tweak dataId handling in RemoteButler.find_dataset

87ae266

Adjusted the handling of dataId and kwargs in RemoteButler.find_datasets to mitigate a few issues:
1. There was a security vulnerability where the caller could smuggle in an arbitrary unvalidated string,  integer or 'None' value to any keyword argument on the server-side 'find_dataset' function
2. Augmentation of DataCoordinate values by kwargs was not working -- it only worked for the 'Mapping' case of DataId
3. It was sending and validating unused DimensionRecords from DataCoordinate.  Removed them to avoid forwards compatibility issues
4. Fixed an issue where 'None' for DataId was not correctly being passed to the server -- it would instead throw a Pydantic error on the client side. None is equivalent to empty dict in find_datasets and get(), so just removed the nullability here.


          Implement kwargs for RemoteButler.get()

4a98858


          Handle more RemoteButler.get StorageClass override

60d47c6

Implement storage class overrides via DatasetRef and DatasetType.


          Implement RemoteButler.get() component override

2921e62


          Move storage class overrides to client side

dba4177

The StorageClass requested by the client may not be known to the server.  There's no need for the server to get involved in StorageClass conversion at all, so it also simplifies the REST API.


          Change get_file to use HTTP GET

47c0957

This doesn't mutate the state of the server and has only a single string parameter, so GET makes sense


          Add missing documentation

166fc17

dhirving force-pushed the tickets/DM-41941 branch from c54ad78 to 166fc17 Compare

December 14, 2023 17:56

dhirving merged commit cc65914 into main

18 checks passed

dhirving deleted the tickets/DM-41941 branch

December 14, 2023 20:22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet