fix: pass integration env vars to Streamlit apps#102
Closed
robertlacok wants to merge 3 commits into
Closed
Conversation
Streamlit processes are started by the installer during bootstrap, before integration env vars are fetched by the Jupyter kernel (in a separate process). The Streamlit subprocess snapshots os.environ at start time, so it never sees the integration env vars. Fetch integration env vars from the webapp endpoint (/integrations/environment-variables) and set them in os.environ before starting Streamlit processes, so the subprocesses inherit them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR fetches integration environment variables from the WebApp /integrations/environment-variables endpoint with retries, validates the JSON list response, and writes validated name/value pairs into os.environ. The Streamlit bootstrap now calls set_integration_env_vars() before discovering Streamlit apps and logs failures without stopping discovery. Unit tests cover fetch success/empty/error/non-list cases and validate setter behavior and skipping of invalid entries. Sequence DiagramsequenceDiagram
participant StreamlitBootstrap as Streamlit Bootstrap
participant WebApp as WebApp Endpoint
participant Installer as Installer
participant OSEnviron as os.environ
participant Subprocess as Streamlit Subprocess
StreamlitBootstrap->>WebApp: GET /integrations/environment-variables (retries)
WebApp-->>Installer: JSON list of {name, value} entries
Installer->>Installer: parse & validate entries
Installer->>OSEnviron: set valid NAME=VALUE pairs
StreamlitBootstrap->>Subprocess: spawn Streamlit subprocess (inherits env)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
robertlacok
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 73.96% 74.06% +0.10%
==========================================
Files 95 95
Lines 5669 5707 +38
Branches 843 849 +6
==========================================
+ Hits 4193 4227 +34
- Misses 1200 1204 +4
Partials 276 276
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@installer/module/streamlit.py`:
- Around line 98-105: Replace the logger.error calls in the exception handlers
with logger.exception so stack traces are recorded: in the except blocks
catching urllib.error.URLError (the handler referencing variable e),
json.JSONDecodeError (handler referencing e), and the generic except Exception
(handler referencing e) used in the Streamlit integration env-var fetch code,
change the logger.error(...) calls to logger.exception(...) while keeping the
existing message strings.
- Around line 93-97: After calling json.loads(json_content) and assigning to
variables, validate that variables is a list before returning; if it's not a
list, log a clear warning via logger (including the unexpected type/value) and
return an empty list so downstream iteration (where variables is used) doesn't
silently drop everything. Reference the variables variable, json_content, and
the existing logger in your change.
In `@tests/unit/test_streamlit.py`:
- Around line 54-55: Add docstrings and explicit return type hints for the new
test class and its methods: update class TestFetchIntegrationEnvVars to include
a one-line class docstring and change def test_fetch_integration_env_vars(self):
to def test_fetch_integration_env_vars(self) -> None: with a one-line docstring
for the test; apply the same pattern (add class/function docstrings and -> None
return hints) to the other new test classes/methods referenced in the review
(the blocks at 79-80, 91-92, 105-106, and 130-131) so all tests follow
repository conventions.
- Around line 126-129: Replace the ad-hoc os.environ.pop cleanup that only runs
after assertions with a failure-safe registration: before invoking the code
under test, register environment teardown using self.addCleanup(lambda:
(os.environ.pop("TEST_INT_VAR_A", None), os.environ.pop("TEST_INT_VAR_B",
None))) or alternatively use unittest.mock.patch.dict to set the test env values
(e.g., patch.dict(os.environ, {"TEST_INT_VAR_A": "...", "TEST_INT_VAR_B":
"..."}, clear=False)) so TEST_INT_VAR_A and TEST_INT_VAR_B are always restored
even if the test fails; update the test method(s) that currently call
os.environ.pop("TEST_INT_VAR_A", None) / os.environ.pop("TEST_INT_VAR_B", None)
to use one of these approaches.
- Around line 131-142: Add a test entry to the mock_data list that includes a
null byte in the name so set_integration_env_vars exercises the name-rejection
branch; specifically add an item like {"name": "BAD\0NAME", "value":
"some_value"} alongside the existing cases in tests/unit/test_streamlit.py so
the set_integration_env_vars function is validated for null bytes in the name
field.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f85937f-c987-41cf-adf4-9a44420d664a
📒 Files selected for processing (3)
installer/__main__.pyinstaller/module/streamlit.pytests/unit/test_streamlit.py
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Validate fetched payload is a list; return [] otherwise - Use logger.exception in fetch handlers for full tracebacks - Make test env cleanup failure-safe via addCleanup - Add docstrings and -> None hints to new tests - Add null-byte-in-name and non-list-payload test cases Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🚀 Review App Deployment Started
|
Contributor
Author
|
This is supported: https://deepnote.com/docs/streamlit#using-integrations |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes an issue where Streamlit apps couldn't access environment variables from integrations.
Root Cause
Streamlit processes are started by the installer during bootstrap (Phase 12 of
installer/__main__.py). At that point, integration env vars haven't been fetched yet — they're only fetched later by the Jupyter kernel'sinit_deepnote_runtime()→set_integration_env(), which runs in a separate process. The Streamlit subprocess snapshotsos.environat start time (ServerProcess._start()callsos.environ.copy()), so it never sees the integration env vars.Fix
Before starting Streamlit processes in the installer, fetch integration env vars from the existing webapp endpoint (
/integrations/environment-variables) and set them inos.environ. This way Streamlit subprocesses inherit the vars.Input validation covers:
isinstancefor both name/value asstr)=, and null bytes in names/values are rejectedlogger.exceptionused in the top-level catch block for full tracebackFiles Changed
installer/module/streamlit.py: Addedfetch_integration_env_vars()andset_integration_env_vars()functions with input validationinstaller/__main__.py: Callset_integration_env_vars()before starting Streamlit appstests/unit/test_streamlit.py: Unit tests for fetch, set, validation, and error handling (including non-dict entries and null-byte values)Summary by CodeRabbit
New Features
Improvements
Tests