Fix nvbug: SSL cert verification and FD exhaustion in email_phishing_analyzer Docker build#1389
Conversation
…r Docker build - Install ca-certificates package and set SSL env vars to fix MacOS SSL errors - Remove --compile-bytecode flag to prevent FD exhaustion on Linux - Add troubleshooting section to README documenting the fixes This resolves Docker build failures on both MacOS (SSLCertVerificationError) and Linux (too many open files during uv sync with bytecode compilation). Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
WalkthroughUpdates the email_phishing_analyzer Dockerfile to install CA certificates, set SSL verification environment variables, and remove the --compile-bytecode flag from uv sync. The README gains a Troubleshooting section covering SSL, Linux build issues, testing steps, and common runtime issues, with duplicated insertion noted. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @examples/evaluation_and_profiling/email_phishing_analyzer/README.md:
- Around line 351-366: The README references a non-existent test script
test_docker_fix.sh; fix by either adding that script or updating the README to
point to existing test entry points: replace the
./examples/.../test_docker_fix.sh invocation with the actual script(s) used for
testing (scripts/run_phishing_evals_all.sh and/or
tests/test_email_phishing_analyzer.py), and update the descriptive steps to
match what those scripts perform (build, start container, test API, check SSL
logs, produce report); ensure the README's command and step list exactly match
the chosen script's behavior and filenames.
🧹 Nitpick comments (1)
examples/evaluation_and_profiling/email_phishing_analyzer/README.md (1)
328-330: Add language specifiers to fenced code blocks.The error message code blocks should have a language specifier for consistency and to satisfy linting rules. Use
textorconsolefor error output.Proposed fix
-``` +```text SSLCertVerificationError: certificate verify failed: unable to get local issuer certificate```diff -``` +```text Failed to build runtime: Os { code: 24, kind: Uncategorized, message: "No file descriptors available" }</details> Also applies to: 343-345 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c55445f27056a47eb29042e8d2019158937d1e1d and e9e942e2027b2610af9fe88e5a4492cad2c885f3. </details> <details> <summary>📒 Files selected for processing (2)</summary> * `examples/evaluation_and_profiling/email_phishing_analyzer/Dockerfile` * `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (6)</summary> <details> <summary>**/*.{md,mdx}</summary> **📄 CodeRabbit inference engine (.cursor/rules/general.mdc)** > `**/*.{md,mdx}`: Use 'NVIDIA NeMo Agent toolkit' for full name (first use), 'NeMo Agent toolkit' or 'the toolkit' for subsequent references, and 'Toolkit' (capital T) in titles/headings, 'toolkit' (lowercase t) in body text > Never use deprecated names: 'Agent Intelligence toolkit', 'aiqtoolkit', 'AgentIQ', 'AIQ', or 'aiq' in documentation; update any occurrences unless intentionally referring to deprecated versions or implementing compatibility layers Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` </details> <details> <summary>**/*.{md,mdx,rst}</summary> **📄 CodeRabbit inference engine (.cursor/rules/general.mdc)** > `**/*.{md,mdx,rst}`: Documentation must be clear, comprehensive, and free of TODOs, FIXMEs, placeholder text, offensive or outdated terms, and spelling mistakes > Do not use words listed in 'ci/vale/styles/config/vocabularies/nat/reject.txt' in documentation > Words listed in 'ci/vale/styles/config/vocabularies/nat/accept.txt' are acceptable even if they appear to be spelling mistakes Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` </details> <details> <summary>**/*.{py,js,ts,tsx,jsx,sh,yaml,yml,json,toml,md,mdx,rst}</summary> **📄 CodeRabbit inference engine (.cursor/rules/general.mdc)** > `**/*.{py,js,ts,tsx,jsx,sh,yaml,yml,json,toml,md,mdx,rst}`: Every file must start with the standard SPDX Apache-2.0 header > Confirm that copyright years are up-to-date whenever a file is changed > All source files must include the SPDX Apache-2.0 header template Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` </details> <details> <summary>**/*.{py,md,mdx,rst}</summary> **📄 CodeRabbit inference engine (.cursor/rules/general.mdc)** > Version numbers are derived automatically by 'setuptools-scm'; never hard-code them in code or docs Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` </details> <details> <summary>**/*</summary> **⚙️ CodeRabbit configuration file** > `**/*`: # Code Review Instructions > - Ensure the code follows best practices and coding standards. - For **Python** code, follow > [PEP 20](https://peps.python.org/pep-0020/) and > [PEP 8](https://peps.python.org/pep-0008/) for style guidelines. > - Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values (except for return values of `None`, > in that situation no return type hint is needed). > Example: > ```python > def my_function(param1: int, param2: str) -> bool: > pass > ``` > - For **Python exception handling**, ensure proper stack trace preservation: > - When re-raising exceptions: use bare `raise` statements to maintain the original stack trace, > and use `logger.error()` (not `logger.exception()`) to avoid duplicate stack trace output. > - When catching and logging exceptions without re-raising: always use `logger.exception()` > to capture the full stack trace information. > > # Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any > words listed in the `ci/vale/styles/config/vocabularies/nat/reject.txt` file, words that might appear to be > spelling mistakes but are listed in the `ci/vale/styles/config/vocabularies/nat/accept.txt` file are OK. > - Documentation in Markdown files should not contain usage of a possessive 's with inanimate objects > (ex: "the system's performance" should be "the performance of the system"). > - Documentation in Markdown files should not use NAT as an acronym, always spell out NeMo Agent Toolkit. > The exception to this rule is when referring to package names or code identifiers that contain "nat", th... Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` - `examples/evaluation_and_profiling/email_phishing_analyzer/Dockerfile` </details> <details> <summary>examples/**/*</summary> **⚙️ CodeRabbit configuration file** > `examples/**/*`: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should > contain a README.md or file README.ipynb. > - If an example contains Python code, it should be placed in a subdirectory named `src/` and should > contain a `pyproject.toml` file. Optionally, it might also contain scripts in a `scripts/` directory. > - If an example contains YAML files, they should be placed in a subdirectory named `configs/`. - If an example contains sample data files, they should be placed in a subdirectory named `data/`, and should > be checked into git-lfs. Files: - `examples/evaluation_and_profiling/email_phishing_analyzer/README.md` - `examples/evaluation_and_profiling/email_phishing_analyzer/Dockerfile` </details> </details><details> <summary>🪛 markdownlint-cli2 (0.18.1)</summary> <details> <summary>examples/evaluation_and_profiling/email_phishing_analyzer/README.md</summary> 328-328: Fenced code blocks should have a language specified (MD040, fenced-code-language) --- 343-343: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)</summary> * GitHub Check: CI Pipeline / Check </details> <details> <summary>🔇 Additional comments (3)</summary><blockquote> <details> <summary>examples/evaluation_and_profiling/email_phishing_analyzer/Dockerfile (2)</summary><blockquote> `31-39`: **LGTM! SSL certificate configuration is well-implemented.** The CA certificates installation follows Docker best practices: - Combines commands in a single RUN layer - Cleans up apt lists to reduce image size - Environment variables correctly point to the standard certificate bundle location --- `56-56`: **LGTM! Removal of `--compile-bytecode` flag addresses the FD exhaustion issue.** This is a reasonable trade-off. Python will compile bytecode on first import, with negligible impact on startup time for this use case. The `PYTHONDONTWRITEBYTECODE=1` set on line 29 ensures the container doesn't write `.pyc` files at runtime anyway. </blockquote></details> <details> <summary>examples/evaluation_and_profiling/email_phishing_analyzer/README.md (1)</summary><blockquote> `322-379`: **LGTM! Troubleshooting section is well-structured and helpful.** The documentation clearly explains the issues and their solutions, provides relevant technical details, and covers common failure scenarios. The content aligns with the Dockerfile changes in this PR. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
examples/evaluation_and_profiling/email_phishing_analyzer/README.md
Outdated
Show resolved
Hide resolved
examples/evaluation_and_profiling/email_phishing_analyzer/README.md
Outdated
Show resolved
Hide resolved
Address reviewer feedback by removing historical bug fix documentation. The fixes mentioned (SSL cert verification and FD exhaustion) are already applied in the Dockerfile, so troubleshooting documentation is not needed. Addresses feedback from willkill07 and CodeRabbit Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release/1.4 #1389 +/- ##
============================================
Coverage 74.95% 74.95%
============================================
Files 553 553
Lines 38833 38833
============================================
Hits 29107 29107
Misses 9726 9726
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…analyzer Docker build (NVIDIA#1389) ## Summary Fixes Docker build failures for the email_phishing_analyzer example on both MacOS and Linux. ## Issues Fixed 1. **MacOS**: `SSLCertVerificationError` during Docker build when downloading packages 2. **Linux**: "Too many open files" error during `uv sync --compile-bytecode` ## Changes - Install `ca-certificates` package and set `REQUESTS_CA_BUNDLE`/`SSL_CERT_FILE` environment variables - Remove `--compile-bytecode` flag from `uv sync` command to prevent file descriptor exhaustion - Add troubleshooting section to README documenting these fixes ## Testing - Successfully built Docker image on MacOS - Container starts and API responds correctly - All CI checks expected to pass (no Python code changes, only Dockerfile and README) ## Description <!-- Note: The pull request title will be included in the CHANGELOG. --> <!-- Provide a standalone description of changes in this PR. --> <!-- Reference any issues closed by this PR with "closes NVIDIA#1234". All PRs should have an issue they close--> Closes ## By Submitting this PR I confirm: - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing/index.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Documentation** * Added comprehensive troubleshooting section to the email phishing analyzer guide covering SSL certificate verification, Docker build failures, and common runtime issues. * **Bug Fixes** * Improved SSL certificate configuration in deployment setup to ensure proper verification during runtime. * Fixed Docker build configuration to resolve file descriptor issues. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Summary
Fixes Docker build failures for the email_phishing_analyzer example on both MacOS and Linux.
Issues Fixed
SSLCertVerificationErrorduring Docker build when downloading packagesuv sync --compile-bytecodeChanges
ca-certificatespackage and setREQUESTS_CA_BUNDLE/SSL_CERT_FILEenvironment variables--compile-bytecodeflag fromuv synccommand to prevent file descriptor exhaustionTesting
Description
Closes
By Submitting this PR I confirm:
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.