Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: fastapi inference server implementation #73
WIP: fastapi inference server implementation #73
Changes from 3 commits
a088ebb
24c200f
7e3d099
b85eea5
c627858
bb0574f
9f41e26
8d2efb1
d3a0bc8
77e11da
0954af9
0f6adfd
0de1609
9cc8380
3f65b4c
42b3d49
657da6a
99282ef
416d085
a7d516a
317740e
5cdb7b0
89550c2
35da357
970fc1f
06c938f
c789d04
0ba4436
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
I'm not sure if we need this new custom Python dependency installer.
There are efforts to move towards more standard way of declaring Python dependencies in Slicer Python scripted modules, while still allowing lazy installation and import. See Allow scripted modules to declare and lazily install pip requirements Slicer/Slicer#7707 and Improving Support for Python Package Dependencies in Slicer Extensions Slicer/Slicer#7171. Therefore, I would not invest time into an alternative way of installing dependencies for one specific extension, but rather spend that time with improving and switching to this new approach.
We could add Slicer-specific improvements that are visible to the user. But I don't see such improvements in this class (it seems pretty much the same functionality as before with slightly different design). For example, if we spend time with improving dependency installation then we could show progress information and errors in a popup window during pip install so that the user knows that something is happening and he has to wait.
If this dependency handler was needed for the uvicorn server then it should be moved there, to make it clear that it is only for that special case.
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.
As long as the Slicer core efforts are not integrated, I am leaning toward using the current version.
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 only works on macOS. Installation command depends on operating system, hardware, and drivers - see the table here: https://pytorch.org/get-started/locally/ To automate it, you need to use a package like light-the-torch. Even with that it is not completely trivial, that's why we have the Slicer pytorch extension. Please always use that extension for installing pytorch.
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 may have been misleading then. The dependency_handler's task is to differentiate between
LocalPythonDependencies
(running fastapi server locally) vsSlicerPythonDependencies
(running webserver within 3D Slicer).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.
Is PyTorch always running in Slicer's Python environment and installed by SlicerPytorch extension?
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.
When server is started from Slicer, SlicerPython will be used. If server is started outside of Slicer, a local environment is required.
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.
PyTorch is installed using the SlicerPytorch extension.
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.
Actually, let me check and I will get back to 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.
@lassoan I made a few changes. When starting the server from Slicer, availability of PyTorch extension will be checked. If successful, other Python packages (monai) will be checked and installed.
As mentioned before, the server can be used independently from Slicer by just running uvicorn from the commandline.