-
Notifications
You must be signed in to change notification settings - Fork 102
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 - part 1 #161
Conversation
… production compose
… routes and multi-route SPAs
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 b5ff66b in 4 minutes and 36 seconds
More details
- Looked at
8154
lines of code in170
files - Skipped
2
files when reviewing. - Skipped posting
23
drafted comments based on config settings.
1. backend/pyspur/nodes/registry.py:64
- Draft comment:
Review the relative module path formatting when registering nodes; prefixing with a dot ('.') may affect imports when packaging. - Reason this comment was not posted:
Comment was on unchanged code.
2. backend/pyproject.toml:21
- Draft comment:
Double-check the dependency 'backend==0.2.4.1'; if this is an internal package, ensure it’s correctly referenced and not conflicting with external packages. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to double-check a dependency, which violates the rule against asking for confirmation or verification. It also involves dependency management, which is not allowed to be commented on unless it's a specific version mismatch issue.
3. .github/workflows/release.yml:41
- Draft comment:
Ensure Dockerfile/backend location is updated to reflect new folder structure (context: '.' and file: './Dockerfile.backend'). - Reason this comment was not posted:
Comment looked like it was already resolved.
4. Dockerfile.backend:30
- Draft comment:
Verify that the frontend build output directory '/pyspur/frontend/out/' matches the actual build output. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to verify something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code.
5. Dockerfile.frontend:3
- Draft comment:
Check that 'COPY frontend/package*.json ./' correctly locates package files in the new structure. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to check if a specific command correctly locates package files in a new structure. This falls under asking the author to double-check or ensure something, which is against the rules. Therefore, this comment should be removed.
6. backend/entrypoint.sh:10
- Draft comment:
The entrypoint now reflects the new package structure; confirm that 'uvicorn "pyspur.api.main:app"' starts the server as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm that a specific command starts the server as expected. This falls under asking the author to confirm their intention or to ensure behavior, which is against the rules.
7. backend/pyproject.toml:21
- Draft comment:
Ensure the dependency 'backend==0.2.4.1' does not conflict with the local module name; review dependency versions. - Reason this comment was not posted:
Marked as duplicate.
8. backend/pyspur/api/main.py:75
- Draft comment:
Consider replacing print statements with proper logging for production use. - Reason this comment was not posted:
Marked as duplicate.
9. backend/pyspur/api/workflow_run.py:181
- Draft comment:
Revisit session management in background tasks; using 'with next(get_db()) as session:' may need more robust handling. - Reason this comment was not posted:
Comment was on unchanged code.
10. backend/pyspur/integrations/slack/client.py:35
- Draft comment:
Use proper logging instead of print statements for error handling in SlackClient. - 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/models/management/alembic/env.py:27
- Draft comment:
Ensure Alembic logging and database URL setup meet production standards. - 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.
12. backend/pyspur/models/management/alembic/versions/000_init_db.py:27
- Draft comment:
Verify that computed columns (e.g., 'DS' || _intid) are supported by your target database. - 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 something, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue with the code. Therefore, it should be removed.
13. backend/pyspur/models/management/alembic/versions/002_add_knowledge_base_model.py:20
- Draft comment:
Migration for 'knowledge_bases' looks standard; confirm it aligns with your intended schema. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm their intention regarding the migration for 'knowledge_bases'. It does not provide a specific suggestion or point out a specific issue, which violates the rules.
14. backend/pyspur/models/management/alembic/versions/003_split_knowledge_base_into_dc_and_vi.py:77
- Draft comment:
Ensure data migration considerations are handled when splitting 'knowledge_bases' into two tables. - 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 data migration considerations are handled, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure something is done. The comment does not provide a specific suggestion or point out a specific issue with the code.
15. backend/pyspur/models/management/alembic/versions/004_add_progress_status.py:23
- Draft comment:
The progress status migration appears appropriate; double-check if any additional indexes are needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to double-check something, which violates the rule against asking the author to confirm or double-check things. It does not provide a specific suggestion or point out a specific issue.
16. backend/pyspur/nodes/factory.py:67
- Draft comment:
Consider adding error handling around config model instantiation in case validation fails. - 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.
17. backend/pyspur/nodes/registry.py:60
- Draft comment:
Verification: Module path adjustment to remove 'pyspur.' prefix is appropriate for consistent imports. - Reason this comment was not posted:
Marked as duplicate.
18. backend/pyspur/schemas/node_type_schemas.py:11
- Draft comment:
NodeTypeSchema properties correctly use import_module with package 'pyspur'; ensure attributes like display_name and logo are set in node classes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that certain attributes are set, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to ensure behavior is intended or tested.
19. docker-compose.prod.yml:1
- Draft comment:
Production Compose now omits the frontend service; confirm intentional removal and validate volume/environment configurations. - Reason this comment was not posted:
Comment was on unchanged code.
20. docker-compose.staging.yml:15
- Draft comment:
Staging Compose configuration appears correct; ensure read-only mounts and volume paths match expectations. - 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 certain configurations match expectations, which violates the rule against asking the author to confirm or ensure things. It doesn't provide a specific suggestion or point out a specific issue.
21. docker-compose.yml:2
- Draft comment:
Development Compose combines nginx, backend, and frontend; verify port mappings and dependency conditions to avoid conflicts. - 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.
22. frontend/next.config.ts:1
- Draft comment:
Next.js config with 'output: export' is suited for static site exports; ensure this aligns with your deployment strategy. - 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 their deployment strategy aligns with the configuration change. It is not making a specific suggestion or pointing out a specific issue with the code. It violates the rule against asking the author to ensure behavior is intended.
23. nginx/conf.d/default.conf:6
- Draft comment:
Nginx config correctly proxies API and frontend requests; verify that 'proxy_pass http://frontend:3000;' is appropriate for your environment. - 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.
Workflow ID: wflow_Xh5NtL04aKsFahXl
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! Incremental review on fd09c47 in 2 minutes and 7 seconds
More details
- Looked at
49
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
3
drafted comments based on config settings.
1. backend/pyspur/api/main.py:75
- Draft comment:
Removed debug print for serving the frontend. If you still need diagnostic info, consider using the logging module instead of print statements in production. - 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%
While the comment is accurate about what changed, it's more of an informative comment explaining the change rather than requesting a specific code change. The print statements were already removed, and the suggestion about logging is more of a general best practice than a specific issue that needs fixing. The comment doesn't identify a problem that needs to be addressed.
The suggestion about using logging could be considered actionable feedback for improving the code quality. Maybe removing debug statements without replacing them with proper logging is a missed opportunity?
The comment is primarily explaining what was already done (removing prints) and making a general suggestion. It doesn't point out a specific issue that needs fixing, and we don't want to keep purely informative comments.
Delete the comment because it's primarily informative and doesn't request a specific code change. The print statements were already removed, and the logging suggestion is too general.
2. backend/pyspur/api/main.py:85
- Draft comment:
Removed multiple print statements for file/directory checks. If logging is required for debugging, integrate a logger rather than using print. - Reason this comment was not posted:
Marked as duplicate.
3. backend/pyspur/api/main.py:75
- Draft comment:
Good cleanup: removed debug print statements. For future debugging, consider using a proper logging framework (e.g., logger.debug) instead of print. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_mzLwQ51SdtUmWQuM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
PySpur PyPI package - Part 1
This PR prepares the codebase to release pyspur python package on PyPI.
In production docker mode, the frontend code is served as static files by the FastAPI server that also serves the APIs.
The docker-compose for this no longer ships with an nginx and f/e service.
Furthermore, the release GitHub action has been updated to not ship frontend image any longer.
./backend/app
directory has been renamed to./backend/pyspur
to prep for the python package release in a follow up PR for this.This pull request includes significant updates to the development environment, Docker configurations, and backend structure. The most important changes include modifications to the
.devcontainer
settings, updates to Docker configurations, and refactoring of backend files and migrations.Development Environment Updates:
.devcontainer/devcontainer.json
: Added new extensionscharliermarsh.ruff
andtamasfe.even-better-toml
, enabled Python linting and formatting withruff
, and added TypeScript settings. [1] [2]Docker Configuration Updates:
.dockerignore
: Added a comprehensive list of files and directories to ignore, including version control, dependencies, build outputs, and more.Dockerfile.backend
: Consolidated Dockerfile for backend, including stages for development, frontend build, and production.Dockerfile.frontend
: Updated and renamedfrontend/Dockerfile
toDockerfile.frontend
, adjusting paths and build instructions. [1] [2]Backend Refactoring:
.env.example
: ChangedNGINX_PORT
toPYSPUR_PORT
for clarity in application port configuration.backend/app/api/main.py
: Removed the entire file, indicating a major refactor or migration of API routes and initialization.backend/app/models/management/alembic/versions/000_init_db.py
andbackend/app/models/management/alembic/versions/002_add_knowledge_base_model.py
: Removed old Alembic migration scripts, suggesting a database schema overhaul. [1] [2]Workflow Adjustments:
.github/workflows/release.yml
: Removed frontend image build and push steps, focusing the workflow on backend image operations. [1] [2]Important
Prepare codebase for PyPI package release by restructuring backend, updating Docker configurations, and modifying development environment.
./backend/app
to./backend/pyspur
for PyPI package preparation.backend/app/api/main.py
and refactored tobackend/pyspur/api/main.py
.backend/pyspur/models/management/alembic/versions/
.Dockerfile.backend
andDockerfile.frontend
for backend and frontend builds.docker-compose.yml
,docker-compose.prod.yml
, anddocker-compose.staging.yml
to reflect new Dockerfile paths and service configurations.docker-compose
..devcontainer/devcontainer.json
with new extensions and settings.pyproject.toml
for project configuration and dependencies..github/workflows/release.yml
to remove frontend image build and focus on backend.nginx/conf.d/default.conf
to adjust API proxy settings.frontend/next.config.ts
to set output to 'export'.This description was created by
for fd09c47. It will automatically update as commits are pushed.