Review of bulk run creation functionality #22

wk9874 · 2025-09-17T08:20:22Z

No description provided.

wk9874

Need to add detail to the README and raise a PR in the docs repo

Also can we add Parquet support?

wk9874 · 2025-09-17T08:21:20Z

pyproject.toml

@@ -87,3 +87,6 @@ lint = [

 [tool.mypy]
 ignore_missing_imports = true
+
+[tool.uv.sources]
+simvue = { git = "https://github.com/simvue-io/python-api", branch = "dev" }


Need to remember to change this back once new PyPI version of Python API released

wk9874 · 2025-09-17T08:22:41Z

src/simvue_cli/actions.py

 from .config import get_url_and_headers
+from .push import PushDelimited


Why are PushJSON and PushDelimited imported differently? Should be consistent

wk9874 · 2025-09-17T08:23:55Z

src/simvue_cli/actions.py

+    )
+
+
+def push_json_metadata(


Surely shouldn't have code repetition here with push_delim_metadata being almost identical except for the class used. Can we use a factory, or just a input_type parameter and if/elif statements to select the appropriate class?

No as this would add a lot of overhead and reduce readability, use of a factory here would be superfluous

wk9874 · 2025-09-17T08:26:22Z

src/simvue_cli/actions.py

+    return _push_class.load_from_metadata(input_file, folder=folder)
+
+
+def push_json_runs(


Again very repetitiive from the above function, can some of this be pulled out into a common setup function which all of these functions use?

Sometimes repetition is the more readable solution, this function is not that long and clearly shows a different reader is used.

But it makes it less maintainable as in the future if we have a lot of file formats and want to add a new param to the CLI interface, we will need to update all of the functions

We already have the if/elif logic in the CLI, based on the suffix of the input file. I dont see why we couldnt have this be one function, where the relevant class is passed in? Would just simplify this here

wk9874 · 2025-09-17T08:29:50Z

src/simvue_cli/cli/__init__.py

@@ -2036,5 +2038,111 @@ def get_artifact_json(ctx, artifact_id: str) -> None:
        click.echo(error_msg, fg="red", bold=True)


+@simvue.group("push")


I'm not sure simvue push is an informative name for this feature

I also think it should be under the run group which you already have, since it is creating a set of runs

simvue run create-batch or something maybe? idk

wk9874 · 2025-09-17T09:18:52Z

src/simvue_cli/push/json.py

+        _folder.commit()
+
+        if not isinstance(_data, list):
+            raise ValueError("Expected JSON content to be a list.")


Should we give the option between a list of dicts, or a dict of dicts? Ie i can see some people may have:

{ "run_1": { "a": 10, "b": 20 }, "run_2": { "a": 15, "b": 25 }, ... }

We could support the key as the run name, and the values as the metadata

This is the JSON format which I would naively expect data to be in instead of a list of dicts

Given that we ourselves can decide this format, and that it is easier if there is just a list of "packets" to process, I would argue enforcing the one form is best here

Tentatively agree, but the point of these loading functions is that it should be as flexible as possible to allow for someone with a file of results not to have to bother fitting it into our format before upload (which I know will never be completely possible)

Exactly, noone is going to happen to have an output that aligns with this anyway, they will have to restructure it regardless (hence the connectors helping)

wk9874 · 2025-09-17T09:20:20Z

src/simvue_cli/push/json.py

+class PushJSON(PushAPI):
+    @pydantic.validate_call
+    def load_from_metadata(
+        self, input_file: pydantic.FilePath, *, folder: str


All same comments as with CSV parser above

wk9874 · 2025-09-17T09:23:19Z

src/simvue_cli/push/core.py

+                )
+            ):
+                if _metrics := self._run_metrics.get(i):
+                    sv_obj.Metrics.new(run=_id, metrics=_metrics).commit()


This will currently not support multi-D metrics right? Do we want to support that?

Not at this stage

wk9874 · 2025-09-17T09:25:45Z

tests/test_command_line_interface.py

+    assert result.exit_code == 0, result.stdout
+
+
+def test_push_runs() -> None:


Should probably set a TTL on these runs, otherwise repeatedly running the tests will very quickly fill up your simvue account!

/ set a folder and delete the folder and runs once complete

wk9874 · 2025-09-17T09:27:48Z

tests/test_command_line_interface.py

+        ],
+        catch_exceptions=False
+    )
+    assert result.exit_code == 0, result.stdout


These tests should use the client to check that the appropriate number of runs have been created, and the correct info is present in at least one of them

kzscisoft added 3 commits September 16, 2025 16:11

✨ Added push commands for metadata as runs, and runs

cb6afb3

📝 [skip ci] Updated CHANGELOG

35107d3

🐛 Added missing files

3ea5658

wk9874 commented Sep 17, 2025

View reviewed changes

wk9874 assigned kzscisoft Sep 17, 2025

		from .config import get_url_and_headers
		from .push import PushDelimited

		return _push_class.load_from_metadata(input_file, folder=folder)


		def push_json_runs(

		@@ -2036,5 +2038,111 @@ def get_artifact_json(ctx, artifact_id: str) -> None:
		click.echo(error_msg, fg="red", bold=True)


		@simvue.group("push")

		assert result.exit_code == 0, result.stdout


		def test_push_runs() -> None:

Review of bulk run creation functionality #22

Are you sure you want to change the base?

Review of bulk run creation functionality #22

Uh oh!

Conversation

wk9874 commented Sep 17, 2025

Uh oh!

wk9874 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!