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

support python 3.11 and fix sqlite initialisation issues #168

Merged
merged 9 commits into from
Feb 21, 2025

Conversation

srijanpatel
Copy link
Collaborator

@srijanpatel srijanpatel commented Feb 21, 2025

This pull request includes several changes to the backend of the PySpur project, focusing on improving compatibility, updating dependencies, and enhancing the code structure.

Compatibility and Dependency Updates:

  • Dockerfile.backend: Downgraded the base image from python:3.12-slim to python:3.10-slim for better compatibility.
  • backend/pyproject.toml: Updated the project version to 0.1.5 and changed the required Python version to >=3.11. Added classifiers for better package categorization.

Code Structure Improvements:

Enhancements and Bug Fixes:

  • backend/pyspur/cli/main.py: Improved .env file handling by checking if PROJECT_ROOT is already defined before appending it.
  • backend/pyspur/cli/utils.py: Enhanced SQLite database initialization by adding more informative print statements and ensuring all models are registered with SQLAlchemy.

Miscellaneous:


Important

Support Python 3.11, fix SQLite initialization, and improve code structure and compatibility in PySpur backend.

  • Compatibility and Dependency Updates:
    • Downgrade Docker base image in Dockerfile.backend from python:3.12-slim to python:3.10-slim.
    • Update Python version requirement to >=3.11 in pyproject.toml.
    • Add classifiers in pyproject.toml for better package categorization.
  • Code Structure Improvements:
    • Reorganize imports in api/main.py and cli/utils.py for readability.
    • Add comments in cli/utils.py to clarify code blocks.
  • Enhancements and Bug Fixes:
    • Improve .env handling in cli/main.py by checking if PROJECT_ROOT is defined before appending.
    • Enhance SQLite initialization in cli/utils.py with informative print statements and ensure all models are registered with SQLAlchemy.
  • Miscellaneous:
    • Reorganize imports in database.py for readability.
    • Add # ruff: noqa: F401 in alembic/env.py to suppress linter warnings.

This description was created by Ellipsis for b4cf0fe. It will automatically update as commits are pushed.

@srijanpatel srijanpatel merged commit 5803e0a into main Feb 21, 2025
1 check passed
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 7447769 in 2 minutes and 27 seconds

More details
  • Looked at 192 lines of code in 7 files
  • Skipped 0 files when reviewing.
  • Skipped posting 10 drafted comments based on config settings.
1. backend/pyproject.toml:128
  • Draft comment:
    Inconsistency in Python versions: mypy target set to 'py312' while the project requires ">=3.11" and Dockerfile uses 3.10.
  • 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.
2. backend/pyspur/api/main.py:60
  • Draft comment:
    Mounting static directories using 'temporary_static_dir' at module load time may always evaluate false since the variable is set during application lifespan.
  • 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.
3. backend/pyspur/models/management/alembic/env.py:82
  • Draft comment:
    Debug print statements for 'render_as_batch' may clutter production logs; consider removing or adjusting the logging level.
  • 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.
4. Dockerfile.backend:1
  • Draft comment:
    The base image uses Python 3.10-slim, but pyproject.toml requires >=3.11. Please align the Docker image's Python version with the project requirements.
  • 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 claims there's a version mismatch with pyproject.toml, we don't actually have access to pyproject.toml contents to verify this claim. The change does lower Python version from 3.12 to 3.10, but without seeing the actual requirements, we can't be certain this is a problem. We should be conservative and not assume requirements we can't verify.
    The comment might be correct - there could be a real version mismatch that would cause issues. The author might have a good reason for downgrading the Python version.
    Without seeing pyproject.toml, we can't verify the claimed version requirement. We should err on the side of not keeping comments when we lack strong evidence.
    Delete the comment because we don't have strong evidence to support the claimed version mismatch with pyproject.toml.
5. backend/pyproject.toml:9
  • Draft comment:
    The project requires Python >=3.11, which conflicts with the Docker image’s Python 3.10. Ensure consistency between the Docker base image and the project's Python version.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    The comment refers to a Docker image that isn't shown in the diff. We can't verify if there actually is a Docker image using Python 3.10. The comment is speculative and asks for verification ("Ensure consistency..."). According to the rules, we shouldn't make speculative comments or ask for verification.
    The comment could be pointing out a real compatibility issue that would cause problems in deployment. Docker configuration is often critical for project functionality.
    While Docker compatibility is important, we don't have evidence of the Docker configuration in this diff. The rules clearly state we should only comment on things we can see strong evidence for, and to not make speculative comments.
    Delete the comment because it's speculative, asks for verification, and refers to Docker configuration that isn't visible in the diff.
6. backend/pyspur/api/main.py:60
  • Draft comment:
    Static file mounts rely on the global 'temporary_static_dir', which is set in the lifespan context but evaluated during module import. This can lead to missing mounts. Consider moving static mount setup into a startup event.
  • 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. backend/pyspur/cli/main.py:61
  • Draft comment:
    When appending PROJECT_ROOT to the .env file, consider using a formatted string (f-string) and atomic file operations to improve readability and ensure consistency.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
8. backend/pyspur/cli/utils.py:131
  • Draft comment:
    Broad exception handling here may hide useful error details. Consider logging the full exception (including stack trace) to aid in debugging.
  • 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.
9. backend/pyspur/models/management/alembic/env.py:82
  • Draft comment:
    Debug print statements are present in the Alembic env file. Consider replacing these with proper logging or removing them before production.
  • 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.
10. backend/pyspur/cli/main.py:78
  • Draft comment:
    Typographical error: 'protoype' should be corrected to 'prototype' in the instruction message.
  • 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_lAPc36tshEe5PuWu


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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 b4cf0fe in 2 minutes and 5 seconds

More details
  • Looked at 10 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. Dockerfile.backend:1
  • Draft comment:
    Check intended Python version: PR description mentioned support for Python 3.11 and compatibility improvements. The base image is now 'python:3.12-slim'. Is this change intended?
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to confirm their intention regarding the Python version change. It is not making a specific code suggestion or asking for a test to be written. This violates the rule against asking the PR author to confirm their intention.
2. Dockerfile.backend:7
  • Draft comment:
    Confirm 'uv' utility usage: 'RUN pip install uv' installs a package 'uv'. Verify it's the intended package and necessary for later steps.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
3. Dockerfile.backend:1
  • Draft comment:
    Base image version conflict: PR details mention downgrading to python:3.10-slim for compatibility, yet this diff uses python:3.12-slim. Please clarify the intended Python version.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
    This is a dependency/version change comment. According to the rules, we should NOT comment on dependency changes or library versions. Additionally, without access to the PR details mentioned, we can't verify if there's actually a conflict. The author likely had a good reason for upgrading to 3.12.
    Maybe there are legitimate compatibility issues that would break the build if we use Python 3.12?
    Even if there are compatibility issues, they would be caught by the build process. The rules explicitly state not to comment on dependency changes.
    Delete this comment as it violates the rule about not commenting on dependency changes and library versions.
4. Dockerfile.backend:7
  • Draft comment:
    Consider pinning the 'uv' package version to ensure consistent builds.
  • 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. Dockerfile.backend:7
  • Draft comment:
    Typographical Error: The command 'pip install uv' on line 7 may contain a typo. If you intended to install a different package (e.g., 'uvicorn' or another utility), please correct the package name accordingly.
  • 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. Dockerfile.backend:10
  • Draft comment:
    Typographical Error: The usage of 'uv pip compile' (line 10) and 'uv pip install' (line 11) appears inconsistent with typical usage of pip commands. If 'uv' is intended to be a prefix (or part of a command), please verify that it is correct, or update it to the intended command (e.g., 'pip-compile' if using pip-tools).
  • 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_y8bWMtKrtlU6XR22


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srijanpatel srijanpatel deleted the refactor/python310 branch February 21, 2025 19:50
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.

1 participant