-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cli #589
Cli #589
Conversation
@bnmajor this is scratch WIP, related:
CC @oeway |
887492b
to
fdecb0c
Compare
@bnmajor could latest updates please be pushed? |
@bnmajor @oeway starting with hypha server infra. @bnmajor initialization, Needs to be run with amun-ai/hypha#432 |
@thewtex amun-ai/hypha#432 is merged now, docs here: https://ha.amun.ai/#/getting-started?id=serve-static-files |
@oeway thank you!! |
@oeway in the latest push, I am getting the errors in the command line console:
why is this and what does it mean? |
It might because of the local proxy was removed when you call a certain function. I just tried your cli branch, the following worked (upgraded ....
await poll_for_services(server)
print('services found')
services = await server.list_services()
service = [s for s in services if s["name"] == "itkwidgets_client"][0]
itk_viewer = await server.get_service(service)
image = itk.imread(data)
await itk_viewer.setImage(np.array(image)) I can see the image get loaded in the opened browser tab. I saw you have some polling functions in the cli.py, maybe a better way to implement it is to register a service in Python, e.g. a image zarr store provider service. We register it before we trigger the browser, when browser is loaded, the viewer use the workspace/token to get the zarr store from the service. Something like this:
In the itk-vtk-viewer, we can get the service and load the remote zarr store, in a passive way. Alternatively, you can also pass the current itk-vtk-viewer api object to the Another advantage is we can have multiple viewer window open at the same time. We can support collaborative visualisation too, since the server service can push viewer parameters to the viewer clients. |
@thewtex @oeway Thank you both for all of your help understanding what we actually need here! This branch has been updated to register services for communication between the client and server rather than registering a plugin and seems to work well for now! There is basic arg support for some of the args the createViewer expects and can be expanded to add support for all of the other kwargs that Additionally there is a convenience function now,
This is achievable through the Python REPL as well but it is a bit more complicated:
|
7365d15
to
b667943
Compare
FYI: i recently added “startup function” support which might be useful for this cli. Not sure how well it will fit the cli case though: https://ha.amun.ai/#/?id=custom-initialization-and-service-integration-with-hypha-server |
Hi @bnmajor I added a synchronous wrapper for imjoy-rpc with hypha server: imjoy-team/imjoy-rpc#546 (It works by running the event loop in a separate thread) |
Thanks @oeway, this is fantastic! With this change I was able to update the function to be synchronous, which means that starting up a basic python interpreter and updating the viewer programmatically is now significantly simpler:
This seems to work well for basic functionality, but if you try to set an image or label image you get the following:
@thewtex The function has also been updated to return a Viewer instance so that all functions are wrapped the same way that they are in the notebook. |
@oeway I did look into using this approach, but it seems that the server object that is returned has no |
itkwidgets/standalone_server.py
Outdated
|
||
loop.create_task(start_viewer(server_url)) | ||
loop.run_forever() | ||
time.sleep(30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Have you tried the latest version, it should work: >>> from imjoy_rpc.hypha import connect_to_server_sync
>>> server = connect_to_server_sync({"server_url": "https://ai.imjoy.io"})
>>> server.register_codec
<bound method RPC.register_codec of <imjoy_rpc.hypha.rpc.RPC object at 0x7fc248082520>> |
BTW, I just added webrtc support for imjoy-rpc: imjoy-team/imjoy-rpc#547 |
Thanks @oeway! That is great to know. What I am actually referring to is the
|
Thanks for all of the updates and fixes! Unfortunately in testing out the functionality a bit more I think that this may not be the move for our use case. We are using the services to trigger the next steps in the pipeline server-side, and this requires sharing some global state variables. Using the the startup function creates a new instance of the module so this global state is not available. If I am maybe missing something here please let me know though! |
@thewtex This PR currently:
|
I see, then I think you are right, startup functions won't work in this case. |
exitmsg = "Exiting REPL. Press CTRL+C to teminate CLI tool." | ||
code.interact(banner=banner, local={"viewer": viewer}, exitmsg=exitmsg) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add:
if __name__ == "__main__":
main()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! But should this be
if __name__ == "__main__":
cli_entrypoint()
instead so that we still parse any flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes :-)
@thewtex The bug that we discussed where we cannot load a label image still exists but it is not obvious why... Thanks to @PaulHax we know that the exact issue is that
@oeway I am leaning towards this being something that I am doing wrong (or fundamentally misunderstanding) rather than an issue in the itk-vtk-viewer code, so if you have any thoughts or ideas I would welcome them! |
@bnmajor great work debugging. Another debugging session with @PaulHax , a few more interesting points:
It appears that there is some issue with our syncified Python RPC that requires JS RPC calls before returning? In any case, is there a workaround that we can use to behave like the initial input data objects, pulling client-side? |
I think what you are observing is related to a dead lock in the syncified python RPC. It happens because we execute the syncified functions in the main thread. If we have a service function A registered, normally, it will respond to remote calls from the js side. However, if we initiate a call from the same main thread to call something on the js side, then the js side try to call A again, the thread gets locked because it will occupy the thread and function A will never get executed. I need to check if I can solve it inside the rpc library. But for now, an easy way to solve this is to create two server connections and use one to register the internal services, and the other one for accessing from the Python REPL. Edit: Fixed now, I increased the worker number in the thread executor of the syncifier and it seems fixes the issue of deadlock. Please upgrade imjoy-rpc to Thanks for reporting! |
itkwidgets/standalone/index.html
Outdated
config: { | ||
visibility: "protected", | ||
require_context: false, | ||
run_in_executor: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't have any effect in js.
I added a commit that adds terminal output by default! 💻 🤓 We can still open in a browser with the
In VSCode: itkwidgets-vscode.webm |
VSCode terminal + tmux. Requires itkwidgets-vscode-tmux.webm |
wezterm on Linux: itkwidgets-wezterm-linux.webm |
Try to pull data from client-side rather than pushing from server-side
A few tweaks are also made to the cli flags for useability. Install: ```sh pip install 'itkwidgets[cli]' playwright install --with-deps chromium ``` Requires a terminal that supports the iterm2 inline image protocal, e.g. wezterm, iterm2, VSCode Terminal.
`import js` will succeed.
@thewtex We can now set images and label images! 🙂
@oeway Thank you for this! I apologize, I have been out for a few days. I did have a chance to try out your changes but unfortunately I was still seeing the same issue. I did end up with a workaround where I am saving the zarr store server-side and then using services to trigger the client-side to pull the store and set the image/label. I found that this exact same approach would fail if I used a service to save the store server-side though (and any attempts to push the store rather than pull it always faill) so I am still not sure that I understand why my current solution actually works. If you have any ideas I would love to understand this better but for now at least we are able to use the full API from the REPL session! Thank you!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 🍾 🎇 💖
WooT! wOOt!
As discussed with @PaulHax , for performance reasons, we are moving to having all the input datasets managed by a DataManager
for the viewer that lives in a WebWorker. In that case, we will have a separate JS event loop in the web worker and separate connections to hypha -- so the workaround we have now will likely be a non-issue.
No description provided.