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

Feat/package - part 2 #162

Merged
merged 6 commits into from
Feb 16, 2025
Merged

Feat/package - part 2 #162

merged 6 commits into from
Feb 16, 2025

Conversation

srijanpatel
Copy link
Collaborator

@srijanpatel srijanpatel commented Feb 16, 2025

PySpur PyPI package release flow

This PR completes the release flow changes required to publish the pyspur python package with each release.


This pull request includes significant updates to the Docker setup, GitHub Actions workflows, and the backend dependencies. The most important changes include switching from requirements.txt to pyproject.toml for Python dependency management, adding new steps to the release workflow, and updating backend dependencies.

Docker setup updates:

  • .devcontainer/Dockerfile: Switched from using requirements.txt to pyproject.toml for installing backend dependencies.
  • Dockerfile.backend: Updated to use pyproject.toml instead of requirements.txt for installing backend dependencies.

GitHub Actions workflow updates:

  • .github/workflows/release.yml: Renamed the workflow from "Release Docker Images" to "Release" and added new jobs for updating the version in pyproject.toml, building and pushing Docker images, and publishing the package to PyPI. [1] [2] [3]

Backend dependencies updates:

  • backend/pyproject.toml: Added new dependencies genanki and praw, and replaced psycopg2 with psycopg2-binary. Removed the commented-out dependency psycopg2. [1] [2] [3]
  • backend/requirements.txt: Removed this file as the project has transitioned to using pyproject.toml for dependency management.

Important

Transitioned to pyproject.toml for dependencies, updated GitHub Actions for PyPI publishing, and modified Docker setup.

  • Dependency Management:
    • Transitioned from requirements.txt to pyproject.toml for backend dependencies in .devcontainer/Dockerfile and Dockerfile.backend.
    • Removed backend/requirements.txt.
  • GitHub Actions:
    • Renamed workflow in release.yml from "Release Docker Images" to "Release".
    • Added update-version, build-and-push-docker, and publish-to-pypi jobs to release.yml for versioning, Docker image handling, and PyPI publishing.
  • Backend Dependencies:
    • Added genanki and praw to backend/pyproject.toml.
    • Replaced psycopg2 with psycopg2-binary in backend/pyproject.toml.

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

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 c7cb822 in 2 minutes and 20 seconds

More details
  • Looked at 211 lines of code in 4 files
  • Skipped 1 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. .github/workflows/release.yml:27
  • Draft comment:
    Using a generic sed regex to update the version in pyproject.toml may be brittle if the file format changes or multiple matching strings exist. Consider using a dedicated TOML parser for robustness.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The comment raises a valid concern about potential brittleness of regex-based file manipulation. However, the current sed pattern is fairly specific and robust for this particular case. The pattern matches exactly what we expect in a pyproject.toml file. While a TOML parser would be more robust, the current solution is simple and likely sufficient for this specific use case.
    The comment identifies a real potential issue with maintainability and robustness. Using proper parsers is generally better practice than regex-based text manipulation.
    While using a TOML parser would be more robust, the current solution is simple, works for the specific case, and the risk of breaking is low given the standardized nature of pyproject.toml files.
    The comment should be deleted because while technically correct, the suggested improvement is not important enough to warrant a code change. The current solution is adequate for this specific use case.
2. .github/workflows/release.yml:100
  • Draft comment:
    Using docker create and exec to build the package may fail if the production image lacks required build tools. Ensure the image includes them or adjust the build process.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment suggests ensuring that the production image includes required build tools or adjusting the build process. This is a general suggestion to ensure compatibility and does not provide a specific code suggestion or ask for a specific test. It falls under the category of asking the author to ensure something is tested or verified, which is against the rules.
3. Dockerfile.backend:10
  • Draft comment:
    Switching to 'pip install .' relies on the pyproject.toml configuration. Confirm that the package metadata and installation instructions are correctly specified for production use.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50%
    None
4. Dockerfile.backend:9
  • Draft comment:
    Only copying 'pyproject.toml' for the pip install in the base stage might omit important package source files. Ensure that the build system (hatchling) is configured to include all necessary files or consider copying additional project files.
  • 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 comment raises a valid concern about pip install . potentially failing without source files. However, this is a multi-stage Dockerfile where the base stage is only used as a foundation for development and production stages. Both subsequent stages (development and production) copy the full backend directory. The base stage likely only needs dependencies installed, not the actual package code.
    I might be wrong about the purpose of the base stage - maybe some operations need the package to be fully installed in the base stage. Also, pip install . behavior might vary depending on the pyproject.toml configuration.
    The multi-stage design clearly shows that the base stage is just for dependencies, with full source code copied in later stages. If there was an issue with missing files, it would fail during build, making this a speculative comment.
    Delete the comment. It raises a speculative issue that would be caught during build if it were real, and the multi-stage design shows intentional separation of dependency installation and source code copying.
5. Dockerfile.backend:28
  • Draft comment:
    In the production stage, the full backend is copied after installation. This might lead to discrepancies between the installed package and the source files. Confirm if reinstallation is needed post-copy or if this layering is intentional.
  • 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/pyproject.toml:21
  • Draft comment:
    The dependency 'backend==0.2.4.1' may unintentionally reference the current project and create a circular dependency. Please verify that this dependency is intended.
  • 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_hqk0MOxzFJcgVJuf


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 fe51476 in 32 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/release.yml:120
  • Draft comment:
    Ensure that changing the packages-dir from backend/dist/ to dist/ aligns with the working directory context. The current working directory is set to ./backend, so dist/ works, but double-check this for consistency with the publish steps.
  • 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 double-check the consistency of the directory change with the publish steps, which violates the rule against asking the author to double-check things. It does not provide a specific suggestion or ask for a specific test to be written, which would be acceptable.
2. .github/workflows/release.yml:119
  • Draft comment:
    The 'packages-dir' value was updated from 'backend/dist/' to 'dist/'. This change correctly aligns with the working directory set to './backend'. Please ensure that the package build step copies the artifacts into './backend/dist' so that the PyPI publish step finds them.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None

Workflow ID: wflow_EERYeapvpoZ4ylaP


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

@srijanpatel srijanpatel merged commit 94443a2 into main Feb 16, 2025
1 check passed
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