refactor(spm): initial refactor of internals to work with new API #114

nfrasser · 2024-12-31T23:07:52Z

Replace CryoSPARC.cli, .vis, .rtp with single .api key
Update implementation of all controller functions to use API
Additional stream methods for convenience
Strategy for mock client in tests

vercel · 2024-12-31T23:07:56Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
cryosparc-tools	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 10, 2025 10:30pm

cryosparc/api.pyi

nfrasser · 2025-01-04T00:15:03Z

cryosparc/dtype.py


 if TYPE_CHECKING:
    from numpy.typing import DTypeLike, NDArray

-Shape = Tuple[int, ...]


Moved to spec.py to avoid unnecessarily importing the native core module when only these simple types are required.

cryosparc/model_registry.py

nfrasser · 2025-01-04T00:31:19Z

I haven't yet completed a full end-to-end tests (re-run the example notebooks) will the real API running, some more changes may come if I find any issues with that. Otherwise the core migration has been completed.

cryosparc/api.py

cryosparc/job.py

cryosparc/tools.py

Puratinamu · 2025-01-06T17:26:18Z

what do you think about adding dataset/ and controller/ as new folders under cryosparc/? The two seem to be more well defined now and having this distinction could help with separation of concerns

Interface remains largely the same, with some deprecations and changes to return values for asset endpoints

Mocks client functions rather than API requests. This is not the greatest, but mocking the HTTP requests would have been way more complicated

nfrasser · 2025-01-07T21:23:33Z

what do you think about adding dataset/ and controller/ as new folders under cryosparc/? The two seem to be more well defined now and having this distinction could help with separation of concerns

@Puratinamu will do for controllers, but how would you do this for datasets?

Something like this?:

dataset.py -> dataset/__init__.py
column.py  -> dataset/column.py
row.py     -> dataset/row.py
dtype.py   -> dataset/dtype.py
core.pyx   -> dataset/core.pyx

cryosparc/job.py

Puratinamu · 2025-01-08T16:45:52Z

what do you think about adding dataset/ and controller/ as new folders under cryosparc/? The two seem to be more well defined now and having this distinction could help with separation of concerns

@Puratinamu will do for controllers, but how would you do this for datasets?

Something like this?:
dataset.py -> dataset/__init__.py
column.py  -> dataset/column.py
row.py     -> dataset/row.py
dtype.py   -> dataset/dtype.py
core.pyx   -> dataset/core.pyx

That sounds good.

Including better changelog

nfrasser · 2025-01-09T22:34:33Z

CHANGELOG.md

@@ -1,5 +1,37 @@
 # Changelog

+## Next


Discussed with Rich , all of these are OK as long as they're well documented

nfrasser · 2025-01-09T22:56:28Z

Ready for another round of review, might have to do another look over the docs closer to deployment to ensure they're still accurate.

Puratinamu

looks good, few more small things

Puratinamu · 2025-01-10T21:41:08Z

cryosparc/tools.py

@@ -83,25 +87,24 @@ class CryoSPARC:
    ``base_port + 5`` should be accessible on the network.


do the port numbers mentioned here have to change with the port number shuffling in SPM?

Good catch, will reword

Puratinamu · 2025-01-10T21:46:30Z

cryosparc/tools.py

+            # TODO: limit find to one workspace
+            workspaces = self.api.workspaces.find(project_uid=[project_uid], order=-1)


why is this TODO not implemented?

The find endpoint doesn't currently accept a limit argument, none of the find endpoints do. They definitely should, but as a larger pagination task.

Puratinamu · 2025-01-10T21:49:52Z

cryosparc/tools.py

+        if workspace_uid is None:
+            # TODO: limit find to one workspace
+            workspaces = self.api.workspaces.find(project_uid=[project_uid], order=-1)
+            workspace = workspaces[0] if workspaces else self.api.workspaces.create(project_uid, title=title)


this makes the title of the workspace the same as the title of the output? Maybe it should be something more generic like "Tools Workspace for <title>"? similarly for the job

We just need some title in a rare case like this, and I'd prefer not to make up a naming convention like this in English - imagining a scenario where someone in Germany records all their project and workspace titles in German, but suddenly sees "Tools Workspace for Ergebnisse der Datenanalyse" or something like that. I know we already do this in some places, but I'd like to minimize this if we don't have to.

Move everything relevant into submodules, as per review feedback at #114 (comment)

nfrasser self-assigned this Dec 31, 2024

vercel bot deployed to Preview December 31, 2024 23:08 View deployment

vercel bot deployed to Preview January 3, 2025 22:38 View deployment

vercel bot deployed to Preview January 3, 2025 23:04 View deployment

vercel bot deployed to Preview January 4, 2025 00:10 View deployment

vercel bot deployed to Preview January 4, 2025 00:14 View deployment

nfrasser commented Jan 4, 2025

View reviewed changes

nfrasser marked this pull request as ready for review January 4, 2025 00:19

vercel bot deployed to Preview January 4, 2025 00:26 View deployment

nfrasser requested review from Puratinamu and notLiria January 4, 2025 00:29

Puratinamu reviewed Jan 6, 2025

View reviewed changes

nfrasser added 7 commits January 7, 2025 11:00

refactor(tools): initial refactor of internals to work with new API

c692b2d

Interface remains largely the same, with some deprecations and changes to return values for asset endpoints

test(tools): update unit tests to work with new api client

ad144b8

Mocks client functions rather than API requests. This is not the greatest, but mocking the HTTP requests would have been way more complicated

fix(tools): stream, dataset and type fixes

b505ee0

fix(tools): python 3.8 correct import

777388b

feat(tools): additional async dump stream helpers

288a6dd

chore(tools): update api and model types

eae5c0a

fix(tools): additional python 3.8 type issues

6127303

fix(tools): review fixes and internal improvements

168f3a3

notLiria reviewed Jan 8, 2025

View reviewed changes

cryosparc/job.py Outdated Show resolved Hide resolved

nfrasser force-pushed the nick/spm/tools-integration branch from 20a6d37 to 168f3a3 Compare January 8, 2025 19:55

vercel bot deployed to Preview January 8, 2025 19:56 View deployment

nfrasser mentioned this pull request Jan 8, 2025

refactor(spm): restructure dataset- and controller- related files into submodules #115

Merged

fix(tools): correct api type names

26547ab

vercel bot deployed to Preview January 9, 2025 16:45 View deployment

nfrasser added 2 commits January 9, 2025 12:12

chore(tools): format with latest ruff

b513509

Merge branch 'spm' into nick/spm/tools-integration

ea34c3a

vercel bot deployed to Preview January 9, 2025 17:22 View deployment

nfrasser added 2 commits January 9, 2025 14:34

refactor(tools): incorportate Rich feedback

d92034d

Including better changelog

refactor(tools): improved stream protocol names

889465b

vercel bot deployed to Preview January 9, 2025 19:39 View deployment

docs(tools): additional docstring updates and improvements

b3db119

vercel bot deployed to Preview January 9, 2025 22:26 View deployment

nfrasser commented Jan 9, 2025

View reviewed changes

refactor(tools): clear_cached_property for convenience

a71f95c

vercel bot deployed to Preview January 9, 2025 22:50 View deployment

docs(tools): correct title/desc defaults

1b82b77

vercel bot deployed to Preview January 9, 2025 22:56 View deployment

nfrasser requested review from notLiria and Puratinamu January 9, 2025 22:56

nfrasser added 2 commits January 10, 2025 14:58

refactor(tools): updated external result API

cfaf043

refactor(tools): file -> asset

884e088

vercel bot deployed to Preview January 10, 2025 20:48 View deployment

Puratinamu reviewed Jan 10, 2025

View reviewed changes

docs(tools): correct CryoSPARC docstring

f1aa968

vercel bot deployed to Preview January 10, 2025 22:30 View deployment

nfrasser requested a review from Puratinamu January 10, 2025 22:31

Puratinamu approved these changes Jan 13, 2025

View reviewed changes

notLiria approved these changes Jan 13, 2025

View reviewed changes

nfrasser merged commit 5c2fc96 into spm Jan 13, 2025
11 checks passed

nfrasser deleted the nick/spm/tools-integration branch January 13, 2025 19:41

nfrasser added a commit that referenced this pull request Jan 13, 2025

refactor(restructure): dataset and controllers submodules (#115)

4cb88d8

Move everything relevant into submodules, as per review feedback at #114 (comment)

nfrasser mentioned this pull request Jan 14, 2025

fix(spm): various bugs in new API integration #118

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(spm): initial refactor of internals to work with new API #114

refactor(spm): initial refactor of internals to work with new API #114

nfrasser commented Dec 31, 2024 •

edited

Loading

vercel bot commented Dec 31, 2024 •

edited

Loading

nfrasser Jan 4, 2025

nfrasser commented Jan 4, 2025

Puratinamu commented Jan 6, 2025 •

edited

Loading

nfrasser commented Jan 7, 2025

Puratinamu commented Jan 8, 2025

nfrasser Jan 9, 2025

nfrasser commented Jan 9, 2025

Puratinamu left a comment

Puratinamu Jan 10, 2025

nfrasser Jan 10, 2025

Puratinamu Jan 10, 2025

nfrasser Jan 10, 2025

Puratinamu Jan 10, 2025

nfrasser Jan 10, 2025

		@@ -83,25 +87,24 @@ class CryoSPARC:
		``base_port + 5`` should be accessible on the network.

		# TODO: limit find to one workspace
		workspaces = self.api.workspaces.find(project_uid=[project_uid], order=-1)

refactor(spm): initial refactor of internals to work with new API #114

refactor(spm): initial refactor of internals to work with new API #114

Conversation

nfrasser commented Dec 31, 2024 • edited Loading

vercel bot commented Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

nfrasser commented Jan 4, 2025

Puratinamu commented Jan 6, 2025 • edited Loading

nfrasser commented Jan 7, 2025

Puratinamu commented Jan 8, 2025

Choose a reason for hiding this comment

nfrasser commented Jan 9, 2025

Puratinamu 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

nfrasser commented Dec 31, 2024 •

edited

Loading

vercel bot commented Dec 31, 2024 •

edited

Loading

Puratinamu commented Jan 6, 2025 •

edited

Loading