fix: skip Streamlit apps whose entrypoint directory no longer exists#109
Open
robertlacok wants to merge 4 commits into
Open
fix: skip Streamlit apps whose entrypoint directory no longer exists#109robertlacok wants to merge 4 commits into
robertlacok wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughTwo guard clauses prevent Streamlit startup against non-existent directories. In Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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!
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![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: 4
🤖 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 `@deepnote_core/execution/registry.py`:
- Around line 250-252: The logger.warning call in the Streamlit app skipping
block is using f-string formatting instead of lazy formatting. Convert the
f-string message to use `%r` placeholders for the action.script and cwd
variables, and pass these variables as separate arguments to the logger.warning
method rather than embedding them in the format string. This ensures compliance
with the G004 linting rule for lazy logger formatting.
In `@installer/module/streamlit.py`:
- Around line 96-98: The logger.warning call around line 96-98 currently uses an
f-string to format the message. Replace the f-string with parameterized
formatting by converting it to a format string with %r placeholders for
entrypoint_path and directory_path, then pass these variables as separate
arguments to the logger.warning method instead of embedding them in the message
string. This ensures the log message is lazy-evaluated and maintains lint
compliance.
In `@tests/unit/test_streamlit.py`:
- Around line 50-54: Add explicit type hints to the test helper functions that
are currently missing them. For the `_make_apps` method, add type annotations
for the `*entrypoints` parameter (variable length arguments) and the return type
(a list of dictionaries). For both `exists_side_effect` functions referenced at
lines 62-63 and 81-82, add type hints for their parameters and return types.
Follow the coding guidelines by ensuring all parameters and return values have
explicit type annotations, making the code more maintainable and consistent with
the rest of the test file.
- Around line 65-66: The nested `with` statements for patching
`installer.module.streamlit.fetch_streamlit_apps` and
`installer.module.streamlit.os.path.exists` should be flattened into a single
`with` statement. Combine both patch calls into one context manager using comma
separation on the same line or across multiple lines with proper continuation,
removing the inner `with` statement indentation and bringing the test logic up
one level to maintain code readability and comply with SIM117. Apply this same
change to all occurrences of nested patch contexts in the test file, including
the instances at lines 84-85.
🪄 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: 626216d1-4440-410c-ada1-5a94c8b51067
📒 Files selected for processing (4)
deepnote_core/execution/registry.pyinstaller/module/streamlit.pytests/unit/test_action_registry.pytests/unit/test_streamlit.py
- Use lazy %r logger formatting (G004) in registry and streamlit module - Fix test_streamlit_with_directory_path to create a real tmp dir so the new directory-existence guard does not skip the action - Flatten nested with-patch blocks (SIM117) in test_streamlit.py - Add type hints to test helpers in test_streamlit.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚀 Review App Deployment Started
|
Contributor
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 `@tests/unit/test_streamlit.py`:
- Line 50: The return type annotation for the _make_apps method uses a generic
`-> list` which lacks specificity. Replace this with a more explicit type hint
such as `-> list[dict]` or `-> list[dict[str, str]]` depending on the actual
structure of dictionaries being returned. This provides clearer type information
to users of the method and aligns with the coding guidelines requiring explicit
type hints.
🪄 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: 568650be-d382-4929-ac8c-feb4df908a30
📒 Files selected for processing (4)
deepnote_core/execution/registry.pyinstaller/module/streamlit.pytests/unit/test_installer_executor.pytests/unit/test_streamlit.py
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/test_streamlit.py (1)
75-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
unittestassertions instead of bareassertinTestCasemethods.Lines 75-79 and 98-99 use bare asserts; switch to
self.assert*for clearer failures and to avoid optimization-mode pitfalls.Suggested patch
- assert any("deleted_folder" in str(a) for a in mock_logger.warning.call_args[0]) - assert mock_venv.start_server.call_count == 1 + self.assertTrue( + any("deleted_folder" in str(a) for a in mock_logger.warning.call_args[0]) + ) + self.assertEqual(mock_venv.start_server.call_count, 1) started_cmd = mock_venv.start_server.call_args[0][0] - assert "existing_folder/app.py" in started_cmd + self.assertIn("existing_folder/app.py", started_cmd) @@ - assert mock_logger.warning.call_count == 2 - assert mock_venv.start_server.call_count == 1 + self.assertEqual(mock_logger.warning.call_count, 2) + self.assertEqual(mock_venv.start_server.call_count, 1)Also applies to: 98-99
🤖 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 `@tests/unit/test_streamlit.py` around lines 75 - 79, Replace bare assert statements with unittest assertion methods throughout the test file. Specifically, change the bare asserts on lines 75-79 and 98-99 to use self.assert* methods: convert assert conditions to self.assertTrue(condition), assert equality comparisons to self.assertEqual(expected, actual), and assert membership checks like "x in y" to self.assertIn(x, y). This provides clearer failure messages and avoids potential issues with Python optimization modes that can disable bare asserts.
♻️ Duplicate comments (1)
tests/unit/test_streamlit.py (1)
50-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten type annotations on newly added test methods.
Line 50 uses a generic
-> list, and Lines 56 and 80 omit return annotations. Prefer explicit types, e.g.-> list[dict[str, str]]for_make_appsand-> Nonefor test methods.As per coding guidelines, use “type hints consistently” and “explicit type hints for function parameters and return values.”
Suggested patch
class TestStartStreamlitServers(unittest.TestCase): - def _make_apps(self, *entrypoints: str) -> list: + def _make_apps(self, *entrypoints: str) -> list[dict[str, str]]: return [ {"id": f"id-{i}", "entrypoint": ep, "port": str(8501 + i), "projectId": "p"} for i, ep in enumerate(entrypoints) ] - def test_skips_app_when_directory_missing(self): + def test_skips_app_when_directory_missing(self) -> None: @@ - def test_continues_after_skipped_app(self): + def test_continues_after_skipped_app(self) -> None:Also applies to: 56-56, 80-80
🤖 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 `@tests/unit/test_streamlit.py` at line 50, The test methods in this file have insufficient type annotations that need to be tightened according to coding guidelines. Update the return type annotation for the _make_apps method on line 50 from the generic `-> list` to a more explicit type such as `-> list[dict[str, str]]` to clearly indicate what the list contains. Additionally, add missing return type annotations of `-> None` to the test methods at lines 56 and 80, as test methods do not return values and should have explicit type hints for consistency with the coding guidelines.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/unit/test_streamlit.py`:
- Around line 75-79: Replace bare assert statements with unittest assertion
methods throughout the test file. Specifically, change the bare asserts on lines
75-79 and 98-99 to use self.assert* methods: convert assert conditions to
self.assertTrue(condition), assert equality comparisons to
self.assertEqual(expected, actual), and assert membership checks like "x in y"
to self.assertIn(x, y). This provides clearer failure messages and avoids
potential issues with Python optimization modes that can disable bare asserts.
---
Duplicate comments:
In `@tests/unit/test_streamlit.py`:
- Line 50: The test methods in this file have insufficient type annotations that
need to be tightened according to coding guidelines. Update the return type
annotation for the _make_apps method on line 50 from the generic `-> list` to a
more explicit type such as `-> list[dict[str, str]]` to clearly indicate what
the list contains. Additionally, add missing return type annotations of `->
None` to the test methods at lines 56 and 80, as test methods do not return
values and should have explicit type hints for consistency with the coding
guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6499c163-f6d9-4063-9c1c-5e79d773af24
📒 Files selected for processing (1)
tests/unit/test_streamlit.py
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.
Fixes a crash where the installer would exit early if a registered Streamlit app's parent directory had been deleted, preventing all subsequent apps from starting.
Summary by CodeRabbit