feat(stream): stream to zenodo-rdm deposit form#13
Conversation
c03fbb6 to
77626f8
Compare
77626f8 to
ceddece
Compare
497936d to
1195026
Compare
| pdf_bytes = response.content | ||
| """Read a file and extract its text content using the specified extractor.""" | ||
| if settings.orcha_env in [Environment.LOCAL, Environment.DEV]: | ||
| with open(request.url, "rb") as f: |
There was a problem hiding this comment.
Maybe we should rename it to request.source as it can now be a locale file path which is not an URL.
There was a problem hiding this comment.
I think we can keep it as it is since the local file path is a temporary thing for local dev
| return | ||
|
|
||
| except SQLAlchemyError as e: | ||
| print("Error in fetching from database (stream_workflow)", e) |
There was a problem hiding this comment.
Maybe we should move to using logging, so we get the stack traces + observability.
c80e161 to
b923845
Compare
| if status == WorkflowStatus.ERROR: | ||
| yield ServerSentEvent( | ||
| # TODO: improve it with a better error message for end users | ||
| data="The Temporal Workflow failed", |
There was a problem hiding this comment.
including translations, if this error will be directly shown in the interface. Otherwise, it is better to return error IDs or similar (numbers?), so that the error msg will be converted by the UI and translated.
bebe3ab to
80919a9
Compare
| select(Workflow).where(Workflow.public_id == workflow_id) | ||
| ).one() | ||
| except NoResultFound: | ||
| raise WorkflowEventError(error_code="WORKFLOW_NOT_FOUND") |
There was a problem hiding this comment.
todo: still need to handle error codes on the ui side
80919a9 to
20d2e76
Compare
|
Failing one test because some changes here depend on #18 (e.g.: Workflow class) |
72dde08 to
7577f4a
Compare
slint
left a comment
There was a problem hiding this comment.
Some minor/nits, but otherwise LGTM
| from sqlalchemy.exc import SQLAlchemyError | ||
| from sqlalchemy.orm.exc import NoResultFound | ||
| from sqlmodel import Session, select | ||
| from sse_starlette import EventSourceResponse, ServerSentEvent |
There was a problem hiding this comment.
minor: I see from the FastAPI SSE docs, you can import these from fastapi.sse. Does this maybe imply also that sse-starlette is already bundled with FastAPI (or comes via an extra?)
There was a problem hiding this comment.
fastapi.sse was introduced in version 0.135.0 and Orcha is on version 0.129.0, which I guess we could bump without issues :) From what I've seen, the implementation doesn't use sse-starlette for the SSE support
There was a problem hiding this comment.
@yashlamba fyi, maybe we can look into using a newer version of fastapi
7577f4a to
e970fce
Compare
❤️ Thank you for your review!
This PR allows integrating the Orcha workflow with InvenioRDM by: