Skip to content
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

feat/init route #5

Merged
merged 5 commits into from
Feb 22, 2024
Merged

feat/init route #5

merged 5 commits into from
Feb 22, 2024

Conversation

kyteinsky
Copy link
Contributor

No description provided.

Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is the call to "path=f'/index.php/apps/app_api/apps/status/{getenv("APP_ID")}'" after each model download?

Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not tested this, but looks fine

Copy link

@andrey18106 andrey18106 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't succeed with local manual install test - looks like the ExApp webserver is hanged after it and /init doesn't happen to report to Nextcloud progress=100.

UPD: Maybe I missed something, let me retry to verify
UPD2: No, unfortunately not working, ocs init dispatch call should be async and not block the python process

@kyteinsky
Copy link
Contributor Author

Didn't succeed with local manual install test - looks like the ExApp webserver is hanged after it and /init doesn't happen to report to Nextcloud progress=100.

yeah noticed that the ocs call wasn't going through. I was looking through the nc_py_api to see how it handled this and seems like the issue is related to ocs call not being async here.

@kyteinsky
Copy link
Contributor Author

made the call async and the url was wrong (maybe old path) :p

@kyteinsky
Copy link
Contributor Author

while we are here, would you mind also testing the persistent storage @andrey18106 ? Thank you!

)

if not app.extra.get('ENABLED', False):
await download_all_models(app, update_progress)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use FastAPI BackgroundTasks in which you will download models in the background

Python's async is a blocking call in this context, reply for "/init" will not be received on NC side..

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, can confirm this issue, finally received time-out error on /init

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that did the trick! How to know if async is blocking in a certain context though?

@kyteinsky kyteinsky force-pushed the master branch 2 times, most recently from df7d7bc to 4ea9789 Compare February 13, 2024 17:48
@kyteinsky kyteinsky linked an issue Feb 14, 2024 that may be closed by this pull request
Signed-off-by: Anupam Kumar <[email protected]>
- use AppAPI's persistent volume
- add /init route and enabled guard
- hash check downloaded model files

Signed-off-by: Anupam Kumar <[email protected]>
Signed-off-by: Anupam Kumar <[email protected]>
USE_COLORS can be used to turn off colors in non-CI envs

Signed-off-by: Anupam Kumar <[email protected]>
@kyteinsky kyteinsky merged commit d834353 into master Feb 22, 2024
3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the feat/init-route branch February 22, 2024 09:59
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.

Nextcloud does not link to install?
3 participants