Mymdc proxy #226

JamesWrigley · 2024-03-18T19:56:03Z

In a nutshell, DAMNIT will now look for mymdc credentials in usr/mymdc-credentials.yml whenever a mymdc#... argument is used in a variable, and use them to retrieve mymdc fields (currently just the sample and run type are allowed).

The credentials file must have the format:

token: <token>
server: <server-url>

Fixes #34.

JamesWrigley · 2024-03-18T20:02:33Z

Oh, and I decided to stick with requests instead of metadata_client because that seemed like a more lightweight dependency, and the code is barely longer than using the metadata_client API anyway.

tmichela · 2024-03-19T14:38:58Z

damnit/ctxsupport/ctxrunner.py

@@ -45,6 +46,45 @@ class DataType(Enum):
    Timestamp = "timestamp"


+def get_run_info(run, run_no, proposal):


I would cache this function, as to not query mymdc multiple times for the same information.

Fixed in 8ce6281.

tmichela · 2024-03-19T14:39:37Z

damnit/ctxsupport/ctxrunner.py

@@ -45,6 +46,45 @@ class DataType(Enum):
    Timestamp = "timestamp"


+def get_run_info(run, run_no, proposal):
+    import yaml
+    import requests


move imports top of the file? Unless there's a good reason not to.

I would guess James has done this because the ctxrunner file is imported in DAMNIT's own frontend & backend processes, as well as the subprocesses executing the context file. By hiding the imports inside functions like this, DAMNIT doesn't require them.

We increasingly run the context file in a separate Python environment from the DAMNIT application - perhaps it's time to make that the default.

Yeah, though with the new class in 8ce6281 it's a little annoying to import everything where necessary so I moved them to the top.

tmichela · 2024-03-19T14:42:15Z

damnit/ctxsupport/ctxrunner.py

+
+    run_info = run_res["runs"][0]
+
+    return headers, server, run_info


I find it shaky to have a function called get_run_info, to return a header with a token and url to be reused in other fonctions in addition to the result it should give (the run info).
I'd split that part in a separate function to do the requests to mymdc (taking the API endpoint as argument).

It could even be a small class, with the server URL & token as instance attributes.

And here I was trying not to make another client 😄 Added a wee one in 8ce6281.

tmichela · 2024-03-19T14:49:05Z

damnit/ctxsupport/ctxrunner.py

+        for name, var in self.vars.items():
+            mymdc_args = var.arg_dependencies("mymdc#")
+            for arg_name, annotation in mymdc_args.items():
+                if annotation not in ["sample", "run_type"]:


nitpicking: I'd call sample sample_name, as the sample data in mymdc may also contains more information.

Renamed in 8ce6281.

takluyver · 2024-03-22T14:27:16Z

damnit/ctxsupport/ctxrunner.py

+    run_res = requests.get(f"{server}/api/mymdc/proposals/by_number/{proposal}/runs/{run_no}",
+                           headers=headers, timeout=MYMDC_TIMEOUT).json()


Suggested change

run_res = requests.get(f"{server}/api/mymdc/proposals/by_number/{proposal}/runs/{run_no}",

headers=headers, timeout=MYMDC_TIMEOUT).json()

resp = requests.get(f"{server}/api/mymdc/proposals/by_number/{proposal}/runs/{run_no}",

headers=headers, timeout=MYMDC_TIMEOUT)

resp.raise_for_status()

run_res = resp.json()

We probably want something like this basically everywhere we make a request - requests doesn't turn HTTP errors into exceptions unless you explicitly ask it to, so if we get something like a 404 or a 401, the failure would be quite confusing.

Good point, fixed in 8ce6281.

JamesWrigley · 2024-03-25T17:19:16Z

Oh, forgot to mention that I added support for propagating changes in the GUI in 323535e.

tmichela

Should we also check if the yaml file exists, and if not ask the proxy to produce one?

damnit/ctxsupport/ctxrunner.py

docs/backend.md

RobertRosca · 2024-03-28T09:50:58Z

Just glanced through the code, I'll leave a review later.

For the caching did you consider just using native HTTP response caching? e.g. https://requests-cache.readthedocs.io/en/stable/

Responses from MyMdC include cache info but always explicitly disable caching, spoke to ITDM about allowing cached responses and they said they'll look into it.

For the proxy I can add the relevant headers to the responses.

JamesWrigley · 2024-03-28T11:15:31Z

Should we also check if the yaml file exists, and if not ask the proxy to produce one?

Excellent point, I forgot about that... added in 1d4a877.

For the caching did you consider just using native HTTP response caching? e.g. https://requests-cache.readthedocs.io/en/stable/

Responses from MyMdC include cache info but always explicitly disable caching, spoke to ITDM about allowing cached responses and they said they'll look into it.

I didn't, but that does look like the way to go 🤔 I'd say let's stick with explicit caching for now until mymdc implements it, I wouldn't dare change the response in the proxy.

JamesWrigley · 2024-04-02T15:11:07Z

(if there's no objections I'll probably merge this at the end of the week)

tmichela · 2024-04-03T11:17:07Z

LGTM

@RobertRosca you said you wanted to have a closer look at that, did you have the chance to do so?

RobertRosca · 2024-04-03T11:29:12Z

I'll have a quick look now

RobertRosca

Some comments but they're minor suggestions, so LGTM 👍

RobertRosca · 2024-04-03T11:46:40Z

damnit/gui/kafka.py

+        # delay in the GUI and because people don't usually close the GUI
+        # immediately after editing a variable so it's unlikely that the process
+        # will die before the update is sent (as happened with the backend).
+        self.kafka_prd.send(self.update_topic, message)


If it's a concern, I guess one option would be to use loop.run_soon, or store a reference to the coroutine and, only when the gui is being closed, await it or .close() it?

Good idea, I added a call to .flush() in fbced4c which should do the same thing.

RobertRosca · 2024-04-03T11:50:45Z

damnit/ctxsupport/ctxrunner.py

+    def __init__(self, proposal, timeout=10, init_server="https://exfldadev01.desy.de/zwop"):
+        self.proposal = proposal
+        self.timeout = timeout
+        self._cache = {}


With how the cache is used (keys as a tuple of arguments basically as far as I could tell?) it seems pretty similar to what using the functools cache decorator would do right?

e.g.

@cache def _run_info(self, run): response = requests.get(f"{self.server}/api/mymdc/proposals/by_number/{self.proposal}/runs/{run}", headers=self._headers, timeout=self.timeout) ....

That's true, but when we make this public the caching should probably be disabled by default (at least until mymdc implements caching headers), so I'll stick with the dictionary for now so we can add an option to disable caching later.

In a nutshell, DAMNIT will now look for mymdc credentials in `usr/mymdc-credentials.yml` whenever a `mymdc#...` argument is used in a variable and use them to retrieve mymdc fields (currently just the sample and run type).

Currently only for the supported message types. Caveat: adding a new editable column doesn't preserve the column position in the update.

JamesWrigley added the enhancement New feature or request label Mar 18, 2024

JamesWrigley requested review from takluyver, RobertRosca and tmichela March 18, 2024 19:56

JamesWrigley self-assigned this Mar 18, 2024

JamesWrigley force-pushed the mymdc-proxy branch from 5a37aa9 to 43ca406 Compare March 18, 2024 21:00

tmichela requested changes Mar 19, 2024

View reviewed changes

takluyver reviewed Mar 22, 2024

View reviewed changes

JamesWrigley force-pushed the mymdc-proxy branch 2 times, most recently from 24b5a36 to 323535e Compare March 24, 2024 06:39

tmichela reviewed Mar 26, 2024

View reviewed changes

damnit/ctxsupport/ctxrunner.py Show resolved Hide resolved

docs/backend.md Outdated Show resolved Hide resolved

docs/backend.md Outdated Show resolved Hide resolved

RobertRosca approved these changes Apr 3, 2024

View reviewed changes

Allow getting different dependency types from arg_dependencies()

eda8074

JamesWrigley force-pushed the mymdc-proxy branch from d499da3 to a5088d1 Compare April 3, 2024 21:11

tmichela approved these changes Apr 4, 2024

View reviewed changes

JamesWrigley added 2 commits April 4, 2024 10:26

Add an integration with mymdc

991e693

In a nutshell, DAMNIT will now look for mymdc credentials in `usr/mymdc-credentials.yml` whenever a `mymdc#...` argument is used in a variable and use them to retrieve mymdc fields (currently just the sample and run type).

Add support for sending updates from the GUI

8c34590

Currently only for the supported message types. Caveat: adding a new editable column doesn't preserve the column position in the update.

JamesWrigley force-pushed the mymdc-proxy branch from fbced4c to 8c34590 Compare April 4, 2024 08:27

JamesWrigley merged commit 5560256 into master Apr 4, 2024
3 checks passed

JamesWrigley deleted the mymdc-proxy branch April 4, 2024 08:30

JamesWrigley mentioned this pull request Apr 16, 2024

Propagate GUI changes to other GUIs #47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mymdc proxy #226

Mymdc proxy #226

JamesWrigley commented Mar 18, 2024

JamesWrigley commented Mar 18, 2024

tmichela Mar 19, 2024

JamesWrigley Mar 25, 2024

tmichela Mar 19, 2024

takluyver Mar 22, 2024

JamesWrigley Mar 25, 2024

tmichela Mar 19, 2024

takluyver Mar 22, 2024

JamesWrigley Mar 25, 2024

tmichela Mar 19, 2024

JamesWrigley Mar 25, 2024

takluyver Mar 22, 2024

JamesWrigley Mar 25, 2024

JamesWrigley commented Mar 25, 2024

tmichela left a comment

RobertRosca commented Mar 28, 2024

JamesWrigley commented Mar 28, 2024

JamesWrigley commented Apr 2, 2024

tmichela commented Apr 3, 2024

RobertRosca commented Apr 3, 2024

RobertRosca left a comment

RobertRosca Apr 3, 2024

JamesWrigley Apr 4, 2024

RobertRosca Apr 3, 2024

JamesWrigley Apr 4, 2024

		@@ -45,6 +46,45 @@ class DataType(Enum):
		Timestamp = "timestamp"


		def get_run_info(run, run_no, proposal):


		run_info = run_res["runs"][0]

		return headers, server, run_info

		run_res = requests.get(f"{server}/api/mymdc/proposals/by_number/{proposal}/runs/{run_no}",
		headers=headers, timeout=MYMDC_TIMEOUT).json()

Mymdc proxy #226

Mymdc proxy #226

Conversation

JamesWrigley commented Mar 18, 2024

JamesWrigley commented Mar 18, 2024

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

JamesWrigley commented Mar 25, 2024

tmichela left a comment

Choose a reason for hiding this comment

RobertRosca commented Mar 28, 2024

JamesWrigley commented Mar 28, 2024

JamesWrigley commented Apr 2, 2024

tmichela commented Apr 3, 2024

RobertRosca commented Apr 3, 2024

RobertRosca 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