AP-677 create tind provider hook by jason-raitz · Pull Request #58 · BerkeleyLibrary/mokelumne

jason-raitz · 2026-05-01T20:56:35Z

Done

version bumped python-tind-client for easier connections without setting default_storage_dir
refactored and removed FetchTind -> /mokelumne/providers/hooks/tind.py
created a provider entrypoint for the new provider
- I slightly changed the ticket to make a mokelumne provider that implements a tind connection-type called tind in the TindHook class. I could be convinced to change this. It was slightly different since many custom providers are assumed to be external imports, not part of the project.
persist the connection in the airflow metastore?
Airflow Connection GUI visible and editable

TODO?

add some customizations like the ones @danschmidt5189 suggested
Possibly rebase and refactor things based on the timing of @awilfox's AP-670 merge

jason-raitz · 2026-05-04T15:36:02Z

In order to persist the Connection in the datastore, we need to add it during the airflow-init container's execution.

If we do it this way, this is what it looks like in the GUI for the Connections page.

And this is what the Connection editor modal looks like for the Tind Connection.

- manually adds Tind API Url and Key to the metastore during airflow-init - allows the user to see and edit the connection in the Airflow GUI

anarchivist

mostly looks good. my main comments are as follows:

i don't know why we're reverting back to the old TIND_API_KEY/TIND_BASE_URL environment variables - it doesn't seem necessary.
there are a lot of unrelated changes to the compose file that seem unnecessary to the PR.
i know we didn't want to add tests for FetchTind but i am curious about what are plans are to add tests for the TINDHook.

anarchivist · 2026-05-04T16:45:04Z

+        if [ -n "${TIND_API_KEY:-}" ]; then
+          airflow connections add tind_default \
+            --conn-type tind \
+            --conn-host "${TIND_BASE_URL}" \
+            --conn-password "${TIND_API_KEY}" \
+            --conn-description "TIND API connection" || \
+          airflow connections set tind_default \
+            --conn-type tind \
+            --conn-host "${TIND_BASE_URL}" \
+            --conn-password "${TIND_API_KEY}"
+        fi


i'm wondering if we actually want to do this. isn't the point to do away with the environment variables altogether (other than AIRFLOW_CONN_TIND_DEFAULT, which was also removed from example.env?)

FWIW I like Jason's method here.

Thoughts:

This is solely about the dev workflow.

Doing it this way retains our simple method: scaffold .env, then run docker compose up --build.

This also retains the ability to modify the connection via the UI, which you lose with AIRFLOW_CONN_TIND_DEFAULT.

I'm honestly struggling to think of a case where we'd want to use AIRFLOW_CONN_TIND_DEFAULT. Any thoughts…?

well, it's in example.env, so my view is that it's optional anyway.

i don't love that we're effectively reverting to requiring an environment variable pair for something that's managed as a connection. using the airflow cli to set it also feels heavy handed.

anarchivist · 2026-05-04T16:46:10Z

 AIRFLOW__API_AUTH__JWT_SECRET=
 # @see https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/fernet.html#generating-fernet-key
 AIRFLOW__CORE__FERNET_KEY=
-AIRFLOW_CONN_TIND_DEFAULT='{"conn_type": "http","password": "your-tind-key-here","host": "https://digicoll.lib.berkeley.edu/api/v1","schema": "https"}'


wait, why are we taking this out?

Per my comment above: if AIRFLOW_CONN_TIND_DEFAULT is set you can't change the value via the UI. @jason-raitz's method is basically the best of both worlds: you still get ENV-based initialization, but you can also change the value later if you want to (i.e. for interactive testing).

anarchivist · 2026-05-04T16:46:31Z

 | `AIRFLOW__API__BASE_URL` | Where Airflow's webserver is reachable. | `AIRFLOW__API__BASE_URL="http://localhost:8080"` |
 | `AIRFLOW__CORE__FERNET_KEY` | [Fernet](https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/fernet.html) encryption key used to encrypt Airflow secrets | `AIRFLOW__CORE__FERNET_KEY="somebase64value="` |
 | `AIRFLOW__API_AUTH__JWT_SECRET` | Secret key used to sign JWT tokens for Airflow's API authentication. The default value used in development and testing should be replaced in production. | `AIRFLOW__API_AUTH__JWT_SECRET="some32bytesecret"` |
-| `AIRFLOW_CONN_TIND_DEFAULT` | Airflow connection json string for TIND access.<br>Note: the Connection params listed in the example are all needed! | `AIRFLOW_CONN_TIND_DEFAULT='{"conn_type": "http","password": "your-tind-key-here","host": "https://digicoll.lib.berkeley.edu/api/v1","schema": "https"}'` |


ditto this?

anarchivist · 2026-05-04T16:49:26Z

general comment - there are a lot of stylistic changes to the compose file in this PR. why are they being included in this PR

I've got a linter installed and actively fixing linting issues on save. I can turn that off....

I second @anarchivist — it's not specific to this PR, and absent agreement within the team and enforcement via CI/tests this sort of change is liable to be unintentionally reverted. I think it's out-of-scope for this PR, but something we could address later via test enforcement.

anarchivist · 2026-05-04T16:51:18Z

    #   langchain-core
+    #   libcst
    #   uvicorn
-pyyaml-ft==8.0.0 \


do we know why this dependency is no longer necessary?

anarchivist · 2026-05-04T16:51:55Z

@@ -0,0 +1,22 @@
+"""Mokelumne Airflow provider."""


Suggested change

"""Mokelumne Airflow provider."""

"""Mokelumne Airflow providers."""

- Airflow 3 prefers provider.yaml for ui field behavior

danschmidt5189

Overall looks very good, I like the direction this is taking. My big question is whether this should be a TIND provider, specifically, rather than "Mokelumne". That makes the most sense to me and is an obvious candidate for extraction / publication down the road. And as I noted in my comment, if we scope this to TIND we should still be able to vendor multiple other custom providers in the ./providers directory by grouping them into subdirs.

danschmidt5189 · 2026-05-05T17:06:51Z

+from typing import Any
+
+
+def get_provider_info() -> dict[str, Any]:


As we noted, this duplicates information already present in the provider.yaml. Looking at community providers it seemed like they either eat that cost (and maintain multiple copies of this info) or use a build script to generate this method based on some static configuration file.

danschmidt5189 · 2026-05-05T17:07:38Z

 AIRFLOW__API_AUTH__JWT_SECRET=
 # @see https://airflow.apache.org/docs/apache-airflow/stable/security/secrets/fernet.html#generating-fernet-key
 AIRFLOW__CORE__FERNET_KEY=
-AIRFLOW_CONN_TIND_DEFAULT='{"conn_type": "http","password": "your-tind-key-here","host": "https://digicoll.lib.berkeley.edu/api/v1","schema": "https"}'


Per my comment above: if AIRFLOW_CONN_TIND_DEFAULT is set you can't change the value via the UI. @jason-raitz's method is basically the best of both worlds: you still get ENV-based initialization, but you can also change the value later if you want to (i.e. for interactive testing).

danschmidt5189 · 2026-05-05T17:09:34Z

@@ -0,0 +1,19 @@
+package-name: mokelumne


Big picture question: Should this be a TIND provider (rather than all of Mokelumne)? That is more tightly scoped, and potentially something we'd want to publish. I could definitely see us adding to that package pretty soon (e.g. TindQueryOperator, TindDownloadOperator, …).

Based on what we saw with community providers, we could still vendor multiple providers using a directory structure like:

./providers/tind/ (this PR) ./providers/mokelumne/ (future)

Where each had its own provider.yaml and get_provider_info() method.

danschmidt5189 · 2026-05-05T17:23:54Z

        try:
-            client = FetchTind.from_connection(orig_run_id, conn="tind_default")
-            filemd = client.get_first_file_metadata(tind_id)
+            hook = TindHook()


Nit: hook seems a little too generic here—maybe tind_hook or just tind?

Aside: I suspect we'll eventually support injecting the tind_conn_id (defaulting to tind_default). We don't quite have a use-case at the moment, but you could imagine that being useful for testing or sandboxing. For example, you could hand a specific collection-scoped TIND connection to student workers.

danschmidt5189 · 2026-05-05T18:13:00Z

+    def __init__(self, conn_id: str = "tind_default") -> None:
+        super().__init__()
+        self.conn_id = conn_id
+        self._client: TINDClient | None = None


Initially it didn't sit right with me that we deferred _client initialization, but based on our review of the community's providers I think that's just a nit. This is all fine.

I'll also correct what I said in our pairing session: Python attributes are read-only by default, so conn_id and _client are effectively tied together and there's no risk of the conn_id being modified by a caller in a confusing way.

danschmidt5189 · 2026-05-05T18:13:38Z

+        self.conn_id = conn_id
+        self._client: TINDClient | None = None
+
+    def get_conn(self) -> TINDClient:


Ditto this — I originally didn't love that this method was called get_conn, implying that it returns a Connection, but after reviewing a few community providers I see that's the standard way of naming this sort of method. LGTM!

danschmidt5189 · 2026-05-05T18:20:44Z

I second @anarchivist — it's not specific to this PR, and absent agreement within the team and enforcement via CI/tests this sort of change is liable to be unintentionally reverted. I think it's out-of-scope for this PR, but something we could address later via test enforcement.

danschmidt5189 · 2026-05-05T18:25:49Z

+        if [ -n "${TIND_API_KEY:-}" ]; then
+          airflow connections add tind_default \
+            --conn-type tind \
+            --conn-host "${TIND_BASE_URL}" \
+            --conn-password "${TIND_API_KEY}" \
+            --conn-description "TIND API connection" || \
+          airflow connections set tind_default \
+            --conn-type tind \
+            --conn-host "${TIND_BASE_URL}" \
+            --conn-password "${TIND_API_KEY}"
+        fi


FWIW I like Jason's method here.

Thoughts:

This is solely about the dev workflow.

Doing it this way retains our simple method: scaffold .env, then run docker compose up --build.

This also retains the ability to modify the connection via the UI, which you lose with AIRFLOW_CONN_TIND_DEFAULT.

I'm honestly struggling to think of a case where we'd want to use AIRFLOW_CONN_TIND_DEFAULT. Any thoughts…?

jason-raitz added 2 commits May 1, 2026 12:16

bump python-tind-client dep

50f2922

basic refactor fetchtind -> TindHook

295f2ec

persist Tind Connection

265d21b

- manually adds Tind API Url and Key to the metastore during airflow-init - allows the user to see and edit the connection in the Airflow GUI

jason-raitz marked this pull request as ready for review May 4, 2026 15:54

jason-raitz requested review from anarchivist, awilfox, danschmidt5189 and yzhoubk May 4, 2026 15:54

anarchivist reviewed May 4, 2026

View reviewed changes

add provider.yaml

c05e100

- Airflow 3 prefers provider.yaml for ui field behavior

danschmidt5189 reviewed May 5, 2026

View reviewed changes

jason-raitz added 4 commits May 6, 2026 11:07

refactoring into tind provider

47becd8

failing tind provider

bccd95a

fix mokelumne providers build

dc5bd86

using pyproject.toml correctly

ed8650e

	"""Mokelumne Airflow provider."""
	"""Mokelumne Airflow providers."""

		from typing import Any


		def get_provider_info() -> dict[str, Any]:

Conversation

jason-raitz commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Done

TODO?

Uh oh!

jason-raitz commented May 4, 2026

Uh oh!

anarchivist left a comment

Choose a reason for hiding this comment

Uh oh!

anarchivist May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anarchivist May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

danschmidt5189 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jason-raitz commented May 1, 2026 •

edited

Loading

anarchivist May 4, 2026 •

edited

Loading

anarchivist May 6, 2026 •

edited

Loading