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

Feature/post eval to self #187

Merged
merged 3 commits into from
Mar 17, 2024
Merged

Feature/post eval to self #187

merged 3 commits into from
Mar 17, 2024

Conversation

emrgnt-cmplxty
Copy link
Contributor

@emrgnt-cmplxty emrgnt-cmplxty commented Mar 17, 2024

Ellipsis 🚀 This PR description was created by Ellipsis for commit 054b7ed.

Summary:

This PR introduces changes in the GitHub workflows, Dockerfile, and Python scripts, including adding a new workflow for building the main branch, renaming an existing workflow, changing the base image in the Dockerfile, modifying the base URL of the API, and adding a file size check in the file upload function.

Key points:

  • Added a new GitHub workflow build-main.yml for building the main branch and triggering templates.
  • Renamed the existing workflow file publish-to-docker.yml to build-release.yml.
  • Changed the base image in the Dockerfile from python:3.9-slim to python:3.9 and simplified the installation command.
  • Changed the base URL of the API in run_client.py from http://localhost:8000 to http://localhost:8010 and commented out several lines of code.
  • Defined a new constant MB_CONVERSION_FACTOR in app.py and added a new condition in the upload_and_process_file function to check if the file size exceeds the maximum allowed size.

Generated with ❤️ by ellipsis.dev

Copy link

vercel bot commented Mar 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
r2r-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 17, 2024 4:01am

@emrgnt-cmplxty emrgnt-cmplxty merged commit 5c56604 into main Mar 17, 2024
1 of 2 checks 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 the entire pull request up to 054b7ed
  • Looked at 378 lines of code in 5 files
  • Took 1 minute and 17 seconds to review
More info
  • Skipped 1 files when reviewing.
  • Skipped posting 3 additional comments because they didn't meet confidence threshold of 50%.
1. Dockerfile:2:
  • Assessed confidence : 50%
  • Comment:
    Consider using a slimmer base image to reduce the Docker image size. If the application doesn't require the full Python image, the slim version could be a better choice.
FROM python:3.9-slim
  • Reasoning:
    The Dockerfile has been changed to use a non-slim version of Python 3.9. This could potentially increase the size of the Docker image. If the slim version was sufficient for the application's needs, it might be better to stick with it to keep the image size down.
2. Dockerfile:13:
  • Assessed confidence : 50%
  • Comment:
    Consider removing the verbose mode for poetry install to reduce the log output. If the verbose mode is not necessary, it might be better to remove it.
RUN poetry install -E parsing -E eval --no-interaction --no-ansi
  • Reasoning:
    The Dockerfile has been changed to use a verbose mode for poetry install. This could potentially increase the log output. If the verbose mode is not necessary, it might be better to remove it to keep the log output minimal.
3. r2r/main/app.py:80:
  • Assessed confidence : 100%
  • Grade: 0%
  • Comment:
    Consider making the maximum file size a configurable setting. This would allow for easy adjustments without changing the code.
if file is not None and file.size > config.app('max_file_size_in_mb', default_value) * MB_CONVERSION_FACTOR:
  • Reasoning:
    The app.py file has a new check for file size during file upload. This is a good practice to prevent large files from being uploaded. However, the maximum file size is hardcoded to 5 MB. It might be better to make this a configurable setting so that it can be easily adjusted without changing the code.

Workflow ID: wflow_jsLUctXTrtBVgNAs


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.


# Example file path for upload
# get file directory
# entry_response = client.add_entry(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider removing the commented out code to improve readability and maintainability of the file. If the commented out code is not necessary, it might be better to remove it.

@emrgnt-cmplxty emrgnt-cmplxty deleted the feature/post-eval-to-self branch March 17, 2024 04:58
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