DM-41162: Create minimalist RemoteButler client and FastAPI server #897

dhirving · 2023-10-23T19:40:41Z

Checklist

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

codecov · 2023-10-23T19:54:38Z

Codecov Report

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

Comparison is base (dd9cc5e) 87.23% compared to head (6f6001b) 87.66%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #897      +/-   ##
==========================================
+ Coverage   87.23%   87.66%   +0.43%     
==========================================
  Files         270      276       +6     
  Lines       36345    36165     -180     
  Branches     7598     7547      -51     
==========================================
+ Hits        31704    31705       +1     
+ Misses       3484     3305     -179     
+ Partials     1157     1155       -2

Files	Coverage Δ
python/lsst/daf/butler/remote_butler/__init__.py	`100.00% <100.00%> (ø)`
python/lsst/daf/butler/remote_butler/_config.py	`100.00% <100.00%> (ø)`
...n/lsst/daf/butler/remote_butler/server/__init__.py	`100.00% <100.00%> (ø)`
...n/lsst/daf/butler/remote_butler/server/_factory.py	`100.00% <100.00%> (ø)`
tests/test_server.py	`91.89% <100.00%> (+66.57%)`	⬆️
.../daf/butler/remote_butler/server/_server_models.py	`0.00% <0.00%> (ø)`
...on/lsst/daf/butler/remote_butler/server/_server.py	`90.90% <90.90%> (ø)`
tests/test_remote_butler.py	`88.23% <88.23%> (ø)`
...on/lsst/daf/butler/remote_butler/_remote_butler.py	`95.83% <95.83%> (ø)`

... and 2 files with indirect coverage changes

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

timj

This reorganization is okay. The main place where I have an issue is how the butler gets configured. This needs fixing but I'm fine with you fixing it on a different ticket since this is not production code. Butler(server_uri) has to work and it doesn't look like it does at the moment (or at least the test code is not demonstrating that)

timj · 2023-10-25T23:06:40Z

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

+        butler_config = ButlerConfig(config, searchPaths, without_datastore=True)
+        self._config = RemoteButlerConfigModel.model_validate(butler_config)
+        self._dimensions: DimensionUniverse | None = None
+        # TODO: RegistryDefaults should have finish() called on it, but this


Yes, we also need to work out what to do with kwargs since people can say instrument="LATISS" and then in the defaults we check that that's a real instrument so we can default it in later queries when there is a need for an instrument. We can decide what to do about that later. It will fold in to how Query objects work and maybe we will pass the defaults to the Query object and let the server sort it out. I don't know if we want to pass kwargs to the server for verification/expansion.

I agree that just passing the defaults to the Query object will probably be the right thing. To stop the execution flow from getting too tangled up between the client and server, I think in general it's going to be the right approach to just record what the user asked for and give it to the server to actually do things with.

timj · 2023-10-25T23:07:02Z

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

+        http_client: httpx.Client | None = None,
+        **kwargs: Any,
+    ):
+        butler_config = ButlerConfig(config, searchPaths, without_datastore=True)


As a note eventually the datastore will be needed.

timj · 2023-10-25T23:10:23Z

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

+        **kwargs: Any,
+    ):
+        butler_config = ButlerConfig(config, searchPaths, without_datastore=True)
+        self._config = RemoteButlerConfigModel.model_validate(butler_config)


Does this validate because ButlerConfig looks like a generic mapping and it happens to have a remote_butler.url in the hierarchy? In general ButlerConfig is not a pydantic model even though we have talked about it being one. What happens to all the other parts of ButlerConfig?

Yes, that's correct. Currently the other parts of ButlerConfig are being discarded since we're not using them. I figured as we added usage of the other parts of ButlerConfig, we could add the appropriate validation for them.

It's still not clear to me which parts of the existing DirectButler ButlerConfig are generated internally in the client code, which are potentially configured locally in a client-side config file, and which are only server-side concepts that the client never sees in a configuration file.

I do wonder if some portion of the configuration would be returned from a versioned "init" endpoint (which maybe gives you back some config chunks, server capabilities, dimensions, collection names, etc all in one shot.) If there is only one "config file" endpoint that configuration format becomes an unversionable permanent part of the API

timj · 2023-10-25T23:12:30Z

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

+        # Docstring inherited.
+        raise NotImplementedError()
+
+    def transaction(self) -> AbstractContextManager[None]:


@TallJimbo I'm assuming transaction shouldn't really be in the ABC?

timj · 2023-10-25T23:15:25Z

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

+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+


Might need to add __all__ to all these files. We do always tend to do the __all__ in the files and then from x import * variant in the __init__.py. I think that it also helps sphinx to know what docs should be built and doesn't get confused by other symbols being imported from elsewhere.

timj · 2023-10-25T23:19:26Z

tests/test_remote_butler.py

+
+    def test_bad_config(self):
+        with self.assertRaises(ValidationError):
+            Butler({"cls": "lsst.daf.butler.remote_butler.RemoteButler", "remote_butler": {"url": "!"}})


Add the if at the end as we do for the other tests so that in theory python tests/test_remote_butler.py does work. Sometimes it's useful to be able to run outside of pytest if that's possible.

timj · 2023-10-25T23:32:04Z

tests/test_server.py

 from lsst.daf.butler.tests.utils import MetricTestRepo, makeTestTempDir, removeTestTempDir

 TESTDIR = os.path.abspath(os.path.dirname(__file__))


+def _make_remote_butler(http_client):


This is not how a normal user is going to create a RemoteButler. They are going to say:

butler = Butler("https://test.example")

and it is meant to work automatically. It's meant to automatically look for the /butler.json endpoint,

It's this code in the prototype server:

app.get("/butler/butler.json", response_model=dict[str, Any]) def read_server_config() -> Mapping: """Return the butler configuration that the client should use.""" config_str = f""" datastore: root: {BUTLER_ROOT} registry: cls: lsst.daf.butler.registries.remote.RemoteRegistry db: <butlerRoot> """ config = Config.fromString(config_str, format="yaml") return config.toDict()

This would clearly need to change to return back the cls. There is a <butlerRoot> convention in the configs that allows the config to say "use what ever URI you found this butler config at" versus a hard-coded server name. So if you go to https://192.168.1.1/butler.json and the server is at https://192.168.1.1 that all works if you set the root field to <butlerRoot> and if you don't like root we can call it server_url or something. Alternatively the butler.json could return a hard-coded server URL that is distinct from where the config came from.

In the test code below that was deleted you will see that it downloads the Config, then injects the httpx client into the config such that the Butler constructor will get the Client object for the server URI rather than having to make it itself from a URI.

We can leave it like this for now but it needs to be fixed to work the standard way soon. Remember that an end user doesn't have to know whether they ended up with a DirectButler or RemoteButler.

By convention in Phalanx/Safir for REST APIs they don't allow external access to things at the top level of the HTTP path -- that's reserved for internal endpoints like Kubernetes health checks. The externally accessible parts are expected to be nested under "/butler/" or something. Not sure if we can override that or not, I can find out.

As it stands you can point the constructor to a config file on disk or an HTTP server and get back a remote butler instance the same way as you would with a direct butler. Whether it makes sense to serve that file from the REST server itself I'm not sure. If it does it may be more like "http://butler.lsst.cloud/configs/dp02" rather than just the root URL.

I kind of think that any configuration chunks that must be provided by the server should come from a versioned endpoint instead of a static file. So if we are keeping the convention from DirectButler that you can load a configuration file from an http server, we might want to do like "butler://butler.lsst.cloud" to mean "create a remote butler pointing at butler.lsst.cloud with all client-side options set to defaults"

andy-slac · 2023-10-26T16:19:03Z

python/lsst/daf/butler/registries/remote.py

I dropped this file also on my branch that has been just merged. Hopefully git will figure it out when you rebase.

Add a no-op RemoteButler that can be instantiated via the Butler() constructor.

Extract dimensions and some httpx setup logic from RemoteRegistry and move them to RemoteButler. Delete RemoteRegistry and tests associated with it.

Instead of manually setting a global variable to configure the test server's Butler, use FastAPI's dependency injection framework.

dhirving force-pushed the tickets/DM-41162 branch 4 times, most recently from 69dfcf9 to fc9af88 Compare October 24, 2023 21:22

dhirving marked this pull request as ready for review October 24, 2023 21:52

timj approved these changes Oct 25, 2023

View reviewed changes

andy-slac reviewed Oct 26, 2023

View reviewed changes

dhirving added 8 commits October 26, 2023 10:53

Add stub RemoteButler

48ecfc8

Add a no-op RemoteButler that can be instantiated via the Butler() constructor.

Move part of RemoteRegistry to RemoteButler

9b608ce

Extract dimensions and some httpx setup logic from RemoteRegistry and move them to RemoteButler. Delete RemoteRegistry and tests associated with it.

Remove unused server code

b6b2975

Move server code into a subpackage

81c29c5

Use FastAPI dependency overrides

f1fbc18

Instead of manually setting a global variable to configure the test server's Butler, use FastAPI's dependency injection framework.

Remove Excalibur branding

3ddba48

Implement collections and run in RemoteButler

d344639

Use __all__ instead of import by name

6f6001b

dhirving force-pushed the tickets/DM-41162 branch from 7eab7d5 to 6f6001b Compare October 26, 2023 17:54

dhirving merged commit e89626a into main Oct 26, 2023
16 checks passed

dhirving deleted the tickets/DM-41162 branch October 26, 2023 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-41162: Create minimalist RemoteButler client and FastAPI server #897

DM-41162: Create minimalist RemoteButler client and FastAPI server #897

dhirving commented Oct 23, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading

timj left a comment

timj Oct 25, 2023

dhirving Oct 26, 2023

timj Oct 25, 2023

timj Oct 25, 2023

dhirving Oct 26, 2023

timj Oct 25, 2023

timj Oct 25, 2023

timj Oct 25, 2023

timj Oct 25, 2023

dhirving Oct 26, 2023

andy-slac Oct 26, 2023

DM-41162: Create minimalist RemoteButler client and FastAPI server #897

DM-41162: Create minimalist RemoteButler client and FastAPI server #897

Conversation

dhirving commented Oct 23, 2023 • edited Loading

Checklist

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

timj 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

dhirving commented Oct 23, 2023 •

edited

Loading

codecov bot commented Oct 23, 2023 •

edited

Loading