-
Notifications
You must be signed in to change notification settings - Fork 195
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/package #163
Feat/package #163
Conversation
…after pyproject.toml commit
… force-include template files
…on port configuration
…ipt for Docker project creation
@ellipsis-dev review this. |
…emplate management
…using importlib.resources
@ellipsis-dev review this |
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.
❌ Changes requested. Reviewed everything up to 95a5a93 in 3 minutes and 18 seconds
More details
- Looked at
1166
lines of code in19
files - Skipped
0
files when reviewing. - Skipped posting
11
drafted comments based on config settings.
1. .github/workflows/release.yml:38
- Draft comment:
Force-updating the release tag (git push --force) can overwrite existing tags—ensure this behavior is intentional. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that the behavior of force-updating a release tag is intentional. It is not making a specific code suggestion or asking for a specific test to be written. It falls under the category of asking the author to confirm their intention, which is against the rules.
2. Dockerfile.backend:7
- Draft comment:
Installing 'uv' via pip is unusual; add a comment clarifying its purpose or ensure it’s version‐pinned. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
- The purpose of 'uv' is evident from its immediate usage in the following lines. 2. It's being used as a build tool to handle Python dependencies. 3. While version pinning could be good practice, this is a dependency management tool itself, not an application dependency. 4. The comment doesn't point out a clear issue that needs fixing.
Version pinning could prevent unexpected breaking changes in the build tool. Also, not everyone might be familiar with 'uv'.
Since 'uv' is a build tool and its purpose is clear from usage, version pinning is less critical here. Documentation for build tools typically belongs in project docs, not Dockerfile comments.
Delete the comment as it doesn't identify a clear issue that needs fixing, and the purpose of 'uv' is self-documenting through its usage.
3. Dockerfile.backend:10
- Draft comment:
Using 'uv' to compile and install dependencies: verify this custom command works as expected and consider handling errors. - Reason this comment was not posted:
Marked as duplicate.
4. backend/pyspur/api/main.py:59
- Draft comment:
Relying on a global 'temporary_static_dir' for static file mounting might be error‐prone; consider mounting these directories within the lifespan context to ensure they are initialized. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. backend/pyspur/api/workflow_run.py:178
- Draft comment:
Avoid using 'with next(get_db()) as session:' as get_db() yields a session and may not support context management properly; use a fresh session instance or correct dependency injection. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. backend/pyspur/api/workflow_run.py:346
- Draft comment:
Similarly, avoid calling 'with next(get_db()) as session:' for session management; consider obtaining a session via SessionLocal() or proper dependency handling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. .github/workflows/release.yml:38
- Draft comment:
Force-updating tags can be risky. Ensure that this behavior is intentional to avoid conflicts with existing releases. - Reason this comment was not posted:
Marked as duplicate.
8. Dockerfile.backend:7
- Draft comment:
The 'uv' package is used here for pip commands; please ensure its purpose is clearly documented as it's not a common tool. - Reason this comment was not posted:
Marked as duplicate.
9. backend/pyspur/api/main.py:59
- Draft comment:
Using a global variable for 'temporary_static_dir' may be fragile. Consider storing this in the app state for clarity and robustness. - Reason this comment was not posted:
Marked as duplicate.
10. backend/pyspur/api/workflow_run.py:178
- Draft comment:
Avoid using 'with next(get_db()) as session:' for session management. Instead instantiate a new session using the sessionmaker context (e.g. 'with SessionLocal() as session:') to ensure proper lifecycle management. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
11. backend/pyspur/cli/main.py:96
- Draft comment:
Consider adding an optional debug or verbose flag for the uvicorn run command to aid troubleshooting during development. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_EciCK2b945Jc1HhW
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.
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.
👍 Looks good to me! Reviewed everything up to db4b592 in 2 minutes and 31 seconds
More details
- Looked at
1329
lines of code in22
files - Skipped
0
files when reviewing. - Skipped posting
23
drafted comments based on config settings.
1. .github/workflows/release.yml:38
- Draft comment:
Force-updating the release tag with 'git tag -f' can be risky. Ensure this behavior is intentional and safe in your release process. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that the behavior is intentional and safe, which violates the rule against asking for confirmation of intention or ensuring safety. It does not provide a specific code suggestion or ask for a specific test to be written.
2. Dockerfile.backend:7
- Draft comment:
Using 'uv pip compile' and 'uv pip install' is unconventional. Verify that 'uv' is the intended package and that these commands work reliably in your build. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify their intention and ensure the commands work reliably, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
3. backend/pyspur/api/main.py:17
- Draft comment:
The use of a global ExitStack and 'temporary_static_dir' outside the lifespan may lead to concurrency issues. Consider managing these resources entirely within the lifespan context. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. backend/pyspur/api/workflow_run.py:177
- Draft comment:
Using 'with next(get_db()) as session:' in an async background task could bypass dependency injection patterns and may not handle session lifecycle correctly. Consider refactoring for proper async session management. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. .env.example:26
- Draft comment:
Good refactoring: centralized app host/port configuration replaces redundant backend/frontend settings. Make sure legacy setups are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. .github/workflows/release.yml:4
- Draft comment:
Changing trigger from 'published' to 'created' and force-updating the tag is risky; verify that forced tag update won’t overwrite important release history. - Reason this comment was not posted:
Marked as duplicate.
7. Dockerfile.backend:10
- Draft comment:
Using 'uv pip compile' for dependency installation is interesting. Consider capturing errors from compilation and ensuring the 'uv' tool remains maintained. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. README.md:84
- Draft comment:
The updated Quick Start guides for both the Python package (Option A) and Docker (Option B) are comprehensive. Ensure all instructions align with current environment variable names. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. backend/pyproject.toml:69
- Draft comment:
New CLI script and build configuration are well integrated. Double-check that ignoring multiple docstring rules in the lint config is intentional. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
10. backend/pyspur/api/api_app.py:21
- Draft comment:
API sub-application routes are set up correctly. Ensure the ordering of router inclusion matches desired URL structure. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
11. backend/pyspur/api/main.py:17
- Draft comment:
Using a global 'temporary_static_dir' in the lifespan is functional but might have concurrency implications. Consider encapsulating this in a class or dependency. - Reason this comment was not posted:
Marked as duplicate.
12. backend/pyspur/api/template_management.py:40
- Draft comment:
Template file handling via importlib.resources is neat. Adding logging for missing templates could aid debugging in production. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
13. backend/pyspur/api/workflow_run.py:177
- Draft comment:
Using 'with next(get_db()) as session:' in background tasks may bypass proper cleanup. Consider explicitly managing the session lifecycle. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. backend/pyspur/cli/main.py:97
- Draft comment:
CLI commands for project initialization and serving are well-structured. Ensure all error cases are clearly handled and logged. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
15. backend/pyspur/cli/utils.py:24
- Draft comment:
Utility functions for file templating, environment loading, and migration are well organized. Error handling appears sufficient. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
16. backend/pyspur/database.py:8
- Draft comment:
Database URL construction and session management are standard. Consider adding checks for missing environment variables to improve error messages. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
17. backend/pyspur/integrations/google/auth.py:32
- Draft comment:
Google token storage is straightforward. Enhancing error logging with more context could help diagnose issues when token storage fails. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
18. backend/pyspur/integrations/google/client.py:30
- Draft comment:
GoogleSheetsClient properly retrieves credentials and calls the Sheets API. Ensure token structure remains compatible with Google’s OAuth updates. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
19. backend/pyspur/models/management/alembic/env.py:76
- Draft comment:
Alembic environment config is clear; printing debug info for render_as_batch is helpful for SQLite migrations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
20. backend/pyspur/utils/path_utils.py:28
- Draft comment:
Path resolution functions are straightforward. Consider normalizing or validating file paths further to handle edge cases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
21. docker-compose.dev.yml:22
- Draft comment:
Development Compose file is well configured with proper volume mappings and health checks. Confirm that mapping the entire project ('.') is intended. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
22. docker-compose.yml:5
- Draft comment:
Production Compose file maps host's PYSPUR_PORT to container’s port 8000; ensure backend entrypoint and port settings match this configuration. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
23. start_pyspur_docker.sh:24
- Draft comment:
The shell script for project initialization is robust with command existence checks and clear messaging. Good use of colors and error handling. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_5ayOTCUCjk0Z4Pqf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This pull request includes significant changes to the configuration, build process, and documentation of the PySpur application. The most important changes involve updates to environment variables, workflow automation, Dockerfile adjustments, and improvements to the API structure and static file handling.
Configuration and Environment Updates:
.env.example
: Added new environment variablesPYSPUR_HOST
andPYSPUR_PORT
, and removed redundant backend and frontend port configurations.Workflow Automation:
.github/workflows/release.yml
: Changed release trigger type frompublished
tocreated
and added a step to update the release tag. [1] [2]Dockerfile Enhancements:
Dockerfile.backend
: Added installation of theuv
package and updated pip installation commands to useuv
for compiling and installing dependencies. Also, added copying of.env.example
to the backend templates directory. [1] [2]API and Static File Handling:
backend/pyspur/api/api_app.py
: Created a sub-application for API routes and included various routers for different functionalities.backend/pyspur/api/main.py
: Refactored to manage application lifespan and static file handling using a temporary directory. [1] [2] [3] [4]Documentation Update:
README.md
: Updated the quick start guide to include new installation and initialization steps for the PySpur Python package and Docker setup.Important
This pull request updates PySpur's configuration, build process, and documentation, enhancing environment setup, workflow automation, Dockerfile, API structure, and static file handling.
PYSPUR_HOST
andPYSPUR_PORT
to.env.example
, removed redundant port configurations.release.yml
frompublished
tocreated
.Dockerfile.backend
, addeduv
package installation and updated pip commands..env.example
to backend templates directory.api_app.py
.main.py
for application lifespan and static file handling.README.md
with new installation and initialization steps for PySpur and Docker setup.This description was created by
for db4b592. It will automatically update as commits are pushed.