fix: split prod/dev requirements to shrink Heroku slug under 1GB#978
fix: split prod/dev requirements to shrink Heroku slug under 1GB#978northdpole wants to merge 1 commit into
Conversation
Move torch/sentence-transformers and importer/test tooling into requirements-dev.txt, keep a slim requirements.txt for Heroku, and lazy-import heavy deps so gunicorn boot no longer pulls them. Closes #975.
northdpole
requested review from
Pa04rth,
paoga87 and
robvanderveer
as code owners
Summary by CodeRabbit
WalkthroughThe PR separates production and development dependencies, redirects development workflows to the expanded dependency set, lazily imports optional modules, adds HTTP 503 handling for unavailable web features, and updates spreadsheet duplicate-header errors and related tests. ChangesDependency and import reduction
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review request for the GSoC Module A–C authors (GitHub only allows formal review requests for collaborators; @Pa04rth is already requested as a reviewer):
Please take a look when you can — especially anything that affects local/CI installs via |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
application/utils/spreadsheet.py (2)
32-33: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale docstring to reflect
ValueErrorinstead ofGSpreadException.The docstring says "Raises
GSpreadExceptionwhen duplicate header names are present" but line 40 now raisesValueError. This misleads callers about the exception contract.📝 Proposed fix
- strings. Raises ``GSpreadException`` when duplicate header names are present + strings. Raises ``ValueError`` when duplicate header names are present🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/spreadsheet.py` around lines 32 - 33, Update the docstring for the affected spreadsheet header-handling function to state that duplicate header names raise ValueError, matching the exception raised at line 40. Remove the stale GSpreadException reference while preserving the rest of the documented behavior.
93-97: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
wshmay be unbound in theexcepthandler if exception is raised before the worksheet loop.The handler at line 96 calls
findDups(wsh.row_values(1)), butwshis only assigned inside thefor wsh in sh.worksheets()loop. If aValueErrororGSpreadExceptionis raised bygspread.oauth(),gspread.service_account(), orgc.open_by_url()— before the loop begins —wshis undefined and the handler itself will raiseUnboundLocalError, masking the original error.Adding
ValueErrorto this catch clause slightly widens the exposure sincegspread.oauth()/gspread.service_account()can raiseValueErrorfor malformed credentials.🛡️ Proposed fix
except (gspread.exceptions.GSpreadException, ValueError) as gse: - logger.error( - "If this exception says you have a duplicate cell name, the duplicate is", - findDups(wsh.row_values(1)), - ) + logger.error("Error reading spreadsheet '%s': '%s'", alias, url) + logger.error(gse) + try: + logger.error( + "If this exception says you have a duplicate cell name, the duplicate is %s", + findDups(wsh.row_values(1)), + ) + except NameError: + pass # wsh not yet assigned if the error occurred before the worksheet loop🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/utils/spreadsheet.py` around lines 93 - 97, Update the exception handler surrounding the spreadsheet setup and worksheet loop so it does not access the loop variable wsh unless a worksheet was successfully assigned; preserve the original GSpreadException or ValueError while logging duplicate-cell details only when wsh is available. Anchor the change to the existing wsh loop and logger.error handler.
🧹 Nitpick comments (1)
application/web/web_main.py (1)
1084-1101: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd regression coverage for unavailable chat dependencies.
Test both the
ImportErrorpath andPromptHandler’s missing-litellmRuntimeErrorpath, asserting HTTP 503. This protects the new production dependency boundary from regressing to a 500 response.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@application/web/web_main.py` around lines 1084 - 1101, Add regression tests for the chat initialization flow in web_main, covering both an ImportError while loading prompt_client and a RuntimeError from PromptHandler indicating missing litellm. Assert each scenario returns HTTP 503 rather than 500, while preserving propagation of unrelated RuntimeError instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@application/prompt_client/prompt_client.py`:
- Around line 232-239: Move the PlaywrightError and PlaywrightTimeoutError
imports in get_content below the _is_likely_pdf_url(url) branch, ensuring PDF
URLs use _fetch_pdf_text_for_embeddings without importing Playwright while
non-PDF paths retain access to those exception types.
In `@requirements.txt`:
- Line 17: Remove the unpinned setuptools entry from the production requirements
list. Keep build and CI tooling dependencies in their existing dedicated
configuration, and only retain setuptools here if Heroku requires it with an
explicitly reviewed version.
In `@scripts/run-local.sh`:
- Around line 15-17: Update the dependency check in the local runner around the
Flask import so it validates the development environment represented by
requirements-dev.txt, not only Flask. Use a development-install marker or
representative dev-only package, or run the idempotent requirements-dev
installation unconditionally, while preserving the existing installation message
and command behavior.
In `@scripts/update-cwe.sh`:
- Around line 18-20: Replace the requests import check in the update-CWE setup
flow with a dedicated development-installation marker, or install
requirements-dev.txt unconditionally; do not use the production dependency as
the readiness signal. Ensure the path invoking cre.py --cwe_in always has all
development dependencies available.
---
Outside diff comments:
In `@application/utils/spreadsheet.py`:
- Around line 32-33: Update the docstring for the affected spreadsheet
header-handling function to state that duplicate header names raise ValueError,
matching the exception raised at line 40. Remove the stale GSpreadException
reference while preserving the rest of the documented behavior.
- Around line 93-97: Update the exception handler surrounding the spreadsheet
setup and worksheet loop so it does not access the loop variable wsh unless a
worksheet was successfully assigned; preserve the original GSpreadException or
ValueError while logging duplicate-cell details only when wsh is available.
Anchor the change to the existing wsh loop and logger.error handler.
---
Nitpick comments:
In `@application/web/web_main.py`:
- Around line 1084-1101: Add regression tests for the chat initialization flow
in web_main, covering both an ImportError while loading prompt_client and a
RuntimeError from PromptHandler indicating missing litellm. Assert each scenario
returns HTTP 503 rather than 500, while preserving propagation of unrelated
RuntimeError instances.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 13ddd54c-327d-406e-8dce-50bd206b7fff
📒 Files selected for processing (17)
.github/workflows/linter.ymlMakefileapplication/cmd/cre_main.pyapplication/prompt_client/prompt_client.pyapplication/tests/spreadsheet_test.pyapplication/tests/web_main_test.pyapplication/utils/external_project_parsers/base_parser.pyapplication/utils/external_project_parsers/base_parser_defs.pyapplication/utils/spreadsheet.pyapplication/web/web_main.pycre.pyrequirements-dev.txtrequirements.txtscripts/build_labeled_dataset.pyscripts/run-local.shscripts/sync_embeddings_table.pyscripts/update-cwe.sh
| def get_content(self, url) -> Optional[str]: | ||
| # Playwright is for scrape/import embedding generation only — not chat. | ||
| from playwright.sync_api import Error as PlaywrightError | ||
| from playwright.sync_api import TimeoutError as PlaywrightTimeoutError | ||
|
|
||
| for attempts in range(1, 10): | ||
| if _is_likely_pdf_url(url): | ||
| text = _fetch_pdf_text_for_embeddings(url) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("application/prompt_client/prompt_client.py")
print("exists:", path.exists())
lines = path.read_text().splitlines()
for start in (210, 220, 230, 240, 250, 260):
end = min(start + 30, len(lines))
print(f"\n--- lines {start}-{end} ---")
for i in range(start, end):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: OWASP/OpenCRE
Length of output: 8470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("application/prompt_client/prompt_client.py"),
Path("pyproject.toml"),
Path("requirements.txt"),
Path("requirements-dev.txt"),
Path("setup.py"),
Path("setup.cfg"),
Path("Pipfile"),
Path("poetry.lock"),
]
for p in files:
if p.exists():
print(f"\n### {p}")
text = p.read_text(errors="ignore").splitlines()
for i, line in enumerate(text, 1):
if "playwright" in line.lower() or "pdf" in line.lower() or "prompt_client" in line.lower():
print(f"{i}: {line}")
PYRepository: OWASP/OpenCRE
Length of output: 4540
Move the Playwright imports below the PDF branch
get_content() imports playwright.sync_api before checking _is_likely_pdf_url(url), so PDF-only calls still fail when Playwright isn’t installed. Move those imports into the non-PDF path to keep PDF extraction lazy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@application/prompt_client/prompt_client.py` around lines 232 - 239, Move the
PlaywrightError and PlaywrightTimeoutError imports in get_content below the
_is_likely_pdf_url(url) branch, ensuring PDF URLs use
_fetch_pdf_text_for_embeddings without importing Playwright while non-PDF paths
retain access to those exception types.
| scikit_learn | ||
| scipy | ||
| semver | ||
| setuptools |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant dependency file and nearby context.
if [ -f requirements.txt ]; then
wc -l requirements.txt
cat -n requirements.txt | sed -n '1,80p'
fi
# Look for references that would justify setuptools as a runtime dependency.
rg -n --hidden --glob '!**/.git/**' '\bsetuptools\b|pkg_resources|distutils' .Repository: OWASP/OpenCRE
Length of output: 2008
Drop setuptools from the production dependency set. It only appears here and in build/CI tooling, so keeping it in requirements.txt pulls build tooling into the runtime image. If Heroku truly needs it, pin it to a reviewed version instead of leaving it unconstrained.
🧰 Tools
🪛 OSV Scanner (2.4.0)
[HIGH] 17-17: setuptools 9.1.0: undefined
(PYSEC-2022-43012)
[HIGH] 17-17: setuptools 9.1.0: undefined
(PYSEC-2025-49)
[HIGH] 17-17: setuptools 9.1.0: setuptools vulnerable to Command Injection via package URL
(PYSEC-2026-1918)
[HIGH] 17-17: setuptools 9.1.0: setuptools has a path traversal vulnerability in PackageIndex.download that leads to Arbitrary File Write
[HIGH] 17-17: setuptools 9.1.0: setuptools vulnerable to Command Injection via package URL
[HIGH] 17-17: setuptools 9.1.0: pypa/setuptools vulnerable to Regular Expression Denial of Service (ReDoS)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@requirements.txt` at line 17, Remove the unpinned setuptools entry from the
production requirements list. Keep build and CI tooling dependencies in their
existing dedicated configuration, and only retain setuptools here if Heroku
requires it with an explicitly reviewed version.
Source: Linters/SAST tools
| if ! python -c "import flask" >/dev/null 2>&1; then | ||
| echo "Installing Python dependencies" | ||
| pip install -r "$ROOT_DIR/requirements.txt" | ||
| pip install -r "$ROOT_DIR/requirements-dev.txt" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check for the development environment, not just Flask.
A pre-existing virtual environment with Flask installed will skip this installation even when the newly separated development dependencies are absent. Use a dev-install marker, check a representative dev-only package, or run the idempotent requirements-dev installation unconditionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run-local.sh` around lines 15 - 17, Update the dependency check in
the local runner around the Flask import so it validates the development
environment represented by requirements-dev.txt, not only Flask. Use a
development-install marker or representative dev-only package, or run the
idempotent requirements-dev installation unconditionally, while preserving the
existing installation message and command behavior.
| if ! python -c "import requests" >/dev/null 2>&1; then | ||
| echo "Installing Python dependencies" | ||
| pip install -r "$ROOT_DIR/requirements.txt" | ||
| pip install -r "$ROOT_DIR/requirements-dev.txt" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use requests as the development-installation readiness check.
Because requests remains in production requirements, an existing environment can pass this check while missing importer dependencies required by cre.py --cwe_in. Use a dedicated dev-install marker or install requirements-dev.txt unconditionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/update-cwe.sh` around lines 18 - 20, Replace the requests import
check in the update-CWE setup flow with a dedicated development-installation
marker, or install requirements-dev.txt unconditionally; do not use the
production dependency as the readiness signal. Ensure the path invoking cre.py
--cwe_in always has all development dependencies available.
1 participant
Summary
requirements.txtfor Heroku/prod (nosentence-transformers/torch), fatrequirements-dev.txtfor local/CI/importers/Librarian ML.prompt_client, playwright/nltk, gspread, alive_progress, OSCAL, coverage) sogunicorn cre:appboots without pulling importer/ML tooling.requirements-dev.txt; Heroku/Dockerfile keep defaultrequirements.txt.Closes #975.
Out of scope
.yarn*,import_telemetry.json)Test plan
make lintpython -m unittest application.tests.web_main_test application.tests.spreadsheet_test/rest/v1/standards,/docs, chat (still has litellm/sklearn on prod in this PR)Made with Cursor