feat: Added temporal codec server route & cookbooks. Updated the CONTRIBUTING.md file #543
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 485e885 in 17 seconds
More details
- Looked at
219lines of code in6files - Skipped
4files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. agents-api/agents_api/routers/internal/router.py:30
- Draft comment:
Thedecode_payloadsfunction should return a JSON response, not aResponseobject. Consider changing the return type hint todictorJSONResponse. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_3vqkZkUCpPNGzqSR
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| # Decode route | ||
| @router.post("/temporal/decode", tags=["temporal"]) | ||
| async def decode_payloads(req: Request)-> Response: |
There was a problem hiding this comment.
The decode_payloads function should return a JSON response, not a Response object. Consider changing the return type hint to dict or JSONResponse.
There was a problem hiding this comment.
we should change return type to -> dict
CONTRIBUTING.md
Outdated
|
|
||
| ```bash | ||
| npm install -g jwt-cli |
There was a problem hiding this comment.
no the lib is different: cargo install jwt-cli --locked
There was a problem hiding this comment.
for cargo installation the users might require extra set installation like Rust and Cargo. Instead can we just ask to install 'jwt-cli' depending on their platform?
cookbooks/03_Cookbook.ipynb
Outdated
| "\n", | ||
| "To recreate the notebook and see the code implementation for this task, you can access the Google Colab notebook using the link below:\n", | ||
| "\n", | ||
| "[](https://drive.google.com/file/d/14ntgAPmm2-QD3KArc5-lyM5wCMyTx5b1/view?usp=drive_link)\n", |
There was a problem hiding this comment.
This isn't the correct link. Use https://openincolab.com/ to generate it but make sure to change the branch name to dev
cookbooks/04_Cookbook.ipynb
Outdated
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "We get the current agent ID from the list of agents." |
There was a problem hiding this comment.
remove the list agents, list tasks, etc. they are useless distractions
cookbooks/04_Cookbook.ipynb
Outdated
| "execution_count": null, | ||
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ |
There was a problem hiding this comment.
mention that list transitions doesnt wait, just lists whatever transitions have happened so far
There was a problem hiding this comment.
and move .list() above .stream()
cookbooks/04_Cookbook.ipynb
Outdated
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "Streaming execution steps of the defined Task." |
There was a problem hiding this comment.
Mention that this is a blocking operation, waits for the execution to get over
|
Also- change names of the cookbook files to something more descriptive like |
|
Also run |
Signed-off-by: Diwank Singh Tomer <[email protected]>
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e8238b2 in 24 seconds
More details
- Looked at
96lines of code in3files - Skipped
4files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. agents-api/agents_api/routers/internal/router.py:15
- Draft comment:
Consider adding error handling for JSON parsing and payload conversion to handle potential exceptions and provide meaningful error responses. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is actionable and suggests a code quality improvement by adding error handling, which is a valid point. The current code does not handle exceptions that might occur during JSON parsing or payload conversion, which could lead to unhandled exceptions and unclear error responses.
The comment does not specify exactly how to implement the error handling, which might make it less actionable. However, the suggestion is clear enough to be useful for a developer to consider adding try-except blocks or similar error handling mechanisms.
While the comment does not provide specific implementation details, it highlights a potential issue that could improve the robustness of the code. This makes it a valuable comment to keep.
Keep the comment as it suggests a valid improvement to the code by adding error handling for potential exceptions.
2. agents-api/agents_api/routers/internal/router.py:14
- Draft comment:
The return type ofdecode_payloadsshould beResponseinstead ofdictto align with FastAPI's expected return types. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change that is not necessary because FastAPI supports returning a dictionary, which it automatically converts to a JSON response. The change in the diff is intentional and aligns with FastAPI's capabilities. Therefore, the comment is not correct as it suggests an unnecessary change.
I might be missing some specific context about the project's coding standards or specific requirements that necessitate usingResponse. However, based on general FastAPI usage, returning a dictionary is acceptable.
The general practice in FastAPI is to allow returning dictionaries for JSON responses, and unless there is a specific project requirement, the current implementation is correct.
The comment should be deleted because it suggests an unnecessary change. The current return type ofdictis valid in FastAPI.
3. CONTRIBUTING.md:71
- Draft comment:
Typo: Replace "fopr" with "for". - Reason this comment was not posted:
Confidence changes required:10%
TheCONTRIBUTING.mdfile contains a minor typo that should be corrected for clarity.
4. CONTRIBUTING.md:89
- Draft comment:
Consider reverting the change to use{JWT_SHARED_KEY}to indicate that it is a placeholder to be replaced by the user. - Reason this comment was not posted:
Confidence changes required:50%
TheCONTRIBUTING.mdfile has an unnecessary change where the curly braces were removed from the JWT_SHARED_KEY placeholder. This could lead to confusion for users.
Workflow ID: wflow_U4Ma66jVM2VvMVfD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Added
/temporal/decoderoute, updated setup instructions inCONTRIBUTING.md, and modified Docker and FastAPI configurations./temporal/decoderoute inrouter.pyfor decoding temporal payloads usingPydanticEncodingPayloadConverter.CONTRIBUTING.mdwith setup instructions, including environment setup, Docker volume creation, and running the project in single or multi-tenant mode.internal.routertoweb.pyto include the new internal routes.docker-compose.ymlto expose port8080foragents-apiandagents-api-multi-tenantservices.This description was created by
for e8238b2. It will automatically update as commits are pushed.