Skip to content

feat: use python binary as a resource instead of a sidecar#74

Merged
sergioramos merged 8 commits into
mainfrom
56-sidecar-binary-error-from-multiprocessingresource_tracker-import-main
Jun 5, 2023
Merged

feat: use python binary as a resource instead of a sidecar#74
sergioramos merged 8 commits into
mainfrom
56-sidecar-binary-error-from-multiprocessingresource_tracker-import-main

Conversation

@gjreda

@gjreda gjreda commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator

This PR no longer creates the python binary in --onefile mode, which is why the binary is so slow, and instead we now use the default --onedir mode.

For this to work with Tauri, we cannot bundle the binary as a sidecar and instead need to add it as a resource.

The first call to the binary is still fairly slow, but subsequent calls are much quicker. We can probably warm up the binary on app startup with a simple "hello world" command.

Open Questions

  • Do we need to change how the interactWithAi command fires, so that we aren't firing so many of them?
  • Do we have to include the target-triple in the filepath? How do we bundle resources for other OS using this pattern? I was only able to get this working by specifying the full path, including the target-triple.
  • Do we need to declare multiple scopes even if the underlying cmd will be the same?
Screen.Recording.2023-06-01.at.2.42.16.PM.mov

@gjreda

gjreda commented Jun 1, 2023

Copy link
Copy Markdown
Collaborator Author

@cguedes @sehyod Can y'all take a look at the video above and front-end changes in this PR? Do we need to change something about how interactWithAi with fires now? So that we aren't firing this command too quickly and getting out of order results?

@gjreda gjreda requested review from cguedes and sergioramos and removed request for cguedes and sergioramos June 1, 2023 21:56
@hammer

hammer commented Jun 2, 2023

Copy link
Copy Markdown
Contributor

Nice!

I was imagining a world where we ship the binary as a sidecar but the shared libraries as resources. With this approach do we lose Tauri’s process supervision? In other words do we need to clean up the resources used by our Python process such as sockets, and do we need to monitor and restart it ourselves if it fails?

Also I agree we should not fire AI interactions on select, we should have a way for the user to trigger the interactions either in the editor or the sidebar or both.

@gjreda

gjreda commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator Author

With this approach do we lose Tauri’s process supervision? In other words do we need to clean up the resources used by our Python process such as sockets, and do we need to monitor and restart it ourselves if it fails?

I don't believe so since we're ultimately still using the same command api, but agree we should get some clarity on the tradeoffs between this approach and the sidecar.

@gjreda gjreda marked this pull request as ready for review June 2, 2023 16:57
@gjreda gjreda requested review from cguedes, hammer and sehyod June 2, 2023 17:48
@gjreda

gjreda commented Jun 2, 2023

Copy link
Copy Markdown
Collaborator Author

Answers from the Tauri team on my questions above:

Do we have to include the target-triple in the filepath? How do we bundle resources for other OS using this pattern? I was only able to get this working by specifying the full path, including the target-triple.

yes and no? Kinda depends on the build pipeline, if building the python app is part of CI and won't be checked into git then you can just use the same folder on all platforms probably. If not, or if there are platform specific configs/changes however that could look like then you probably need a similar approach.
Btw there are platform specific tauri config files you can use to have the same cmd on all platforms using different folders: https://tauri.app/v1/api/config#platform-specific-configuration

Since we are all using Macs and not checking in the binary, we will likely need CI/CD to build it for us anyway. So in this case, we shouldn't have to worry about the target-triple and should be able to have the following cmd path in tauri.conf for all OS:

        {
          "name": "ingest-pdfs",
          "cmd": "$RESOURCE/bin/python/main/main",
          "args": true
        }

Does using the binary as a resource vs a sidecar change anything about how we have to manage the underlying processes? Do we need to clean up the resources used by our Python process such as sockets, and do we need to monitor and restart it ourselves if it fails?

a very valid question! But there is no difference in the behavior/cleanup as long as you use tauri's Command/Child apis
That said, i still suggest having a close eye on the cleanup because tauri only kills the main sidecar process but doesn't touch its children. For most of the sidecars i've seen so far this wasn't a problem, but for pyinstaller in single file mode it actually was, because it uses a wrapper process which doesn't notify the child, nor does the child check if the parent still exists...

In that case i tend to recommend https://docs.rs/command-group/latest/command_group/ with which you'll have to do the cleanup yourself but at least all sidecar processes should be killed. iirc this wasn't necessary for pyinstaller's folder mode but i didn't see it in use with sidecars that big so i'm not 100% sure

@sergioramos sergioramos changed the title Use python binary as a resource instead of a sidecar (#56) feat: use python binary as a resource instead of a sidecar Jun 5, 2023
@sergioramos sergioramos merged commit 99ddba6 into main Jun 5, 2023
@sergioramos sergioramos deleted the 56-sidecar-binary-error-from-multiprocessingresource_tracker-import-main branch June 5, 2023 10:38
@gjreda gjreda mentioned this pull request Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sidecar dylib binary errors: PyInstaller --onedir does not play nicely with Tauri sidecar pattern

4 participants