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
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 Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
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. Co-authored-by: Cursor <cursoragent@cursor.com>
northdpole
force-pushed
the
fix/heroku-requirements-split
branch
from
5b7b6f6 to
a58dd33
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/spreadsheet.py`:
- Around line 94-99: Update the duplicate-header error log inside the exception
handler to include a `%s` placeholder in the message passed to `logger.error`,
while preserving the existing `findDups(wsh.row_values(1))` argument and
surrounding behavior.
🪄 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: ff0120ce-135f-4503-9b8d-cbc3b7a3c90c
📒 Files selected for processing (18)
.github/workflows/linter.ymlMakefileapplication/cmd/cre_main.pyapplication/prompt_client/prompt_client.pyapplication/tests/chat_completion_test.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
🚧 Files skipped from review as they are similar to previous changes (13)
- scripts/build_labeled_dataset.py
- scripts/run-local.sh
- .github/workflows/linter.yml
- scripts/sync_embeddings_table.py
- Makefile
- application/utils/external_project_parsers/base_parser_defs.py
- application/utils/external_project_parsers/base_parser.py
- requirements-dev.txt
- application/tests/web_main_test.py
- cre.py
- application/web/web_main.py
- application/prompt_client/prompt_client.py
- application/tests/spreadsheet_test.py
| except (gspread.exceptions.GSpreadException, ValueError) as gse: | ||
| if wsh is not None: | ||
| logger.error( | ||
| "If this exception says you have a duplicate cell name, the duplicate is", | ||
| findDups(wsh.row_values(1)), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix logging format string mismatch on the duplicate-header error path.
The format string "If this exception says you have a duplicate cell name, the duplicate is" has no %s placeholder, but findDups(wsh.row_values(1)) is passed as a logging argument. At runtime, Python's logging will attempt "…" % (findDups(…),) and raise TypeError: not all arguments converted during string formatting. The logging framework swallows this via handleError, so no log message is emitted when a duplicate-header ValueError is caught — the exact scenario this PR newly introduces into the handler.
Ruff also flags this as PLE1205 at line 96.
🐛 Proposed fix: add %s to the format string
except (gspread.exceptions.GSpreadException, ValueError) as gse:
if wsh is not None:
logger.error(
- "If this exception says you have a duplicate cell name, the duplicate is",
+ "If this exception says you have a duplicate cell name, the duplicate is %s",
findDups(wsh.row_values(1)),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except (gspread.exceptions.GSpreadException, ValueError) as gse: | |
| if wsh is not None: | |
| logger.error( | |
| "If this exception says you have a duplicate cell name, the duplicate is", | |
| findDups(wsh.row_values(1)), | |
| ) | |
| except (gspread.exceptions.GSpreadException, ValueError) as gse: | |
| if wsh is not None: | |
| logger.error( | |
| "If this exception says you have a duplicate cell name, the duplicate is %s", | |
| findDups(wsh.row_values(1)), | |
| ) |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 96-96: Too many arguments for logging format string
(PLE1205)
🤖 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 94 - 99, Update the
duplicate-header error log inside the exception handler to include a `%s`
placeholder in the message passed to `logger.error`, while preserving the
existing `findDups(wsh.row_values(1))` argument and surrounding behavior.
Source: Linters/SAST tools
|
Hi @northdpole took a look from the Module C (Librarian) side. Dropping sentence-transformers / torch from the slim prod requirements.txt is fine for us too, since the Librarian isn't actually run by the web server yet (it's opt-in CLI / dry-run) and the cross-encoder import is lazy, so prod won't try to load them at boot. I'll re-verify locally once it's merged. |
|
hi @northdpole, took a look from the Module A (Harvester) side. From what I can see, nothing here changes the harvester pipeline itself, and the dependency split doesn’t affect the parser/retriever/normalizer flow. Since the harvester only relies on the runtime dependencies that remain in the slim requirements.txt, moving the importer/ML tooling into requirements-dev.txt shouldn’t impact Module A. The lazy imports also don’t appear to touch any of the diff pipeline code paths. I also took a quick look through the lazy import changes. Overall they look good and I don’t see any obvious missing imports or circular import issues. The new tests around the optional dependency handling are a nice addition as well. Only one minor observation :
Otherwise, from the Module A side this LGTM. I’ll re verify after merge as well, but I don’t see anything that should affect the harvester. |
3 participants
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