Require HTTP polling for GitHub Codespaces context #5349
Conversation
WalkthroughThe changes introduce a new function Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
fiftyone/core/context.py (2)
144-153: Implementation looks good with minor optimization possible.The function correctly implements the polling requirements check for both Codespaces and Colab environments. Consider simplifying the Colab check:
- if _get_context() == _COLAB: - return True - - return False + return _get_context() == _COLAB🧰 Tools
🪛 Ruff (0.8.2)
150-153: Return the condition
_get_context() == _COLABdirectlyReplace with
return _get_context() == _COLAB(SIM103)
181-182: Consider adding a comment explaining the polling parameter.The condition for enabling polling is clear, but it would be helpful to document why polling is required when using a proxy or in certain environments. Consider adding a comment similar to the ones in
_requires_http_polling():+ # Enable HTTP polling when using a proxy or in environments where SSE is unreliable if "proxy" in kwargs or _requires_http_polling(): kwargs["polling"] = "true"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/core/context.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/core/context.py
150-153: Return the condition _get_context() == _COLAB directly
Replace with return _get_context() == _COLAB
(SIM103)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: lint / eslint
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-app
- GitHub Check: build
🔇 Additional comments (1)
fiftyone/core/context.py (1)
144-153: Consider adding unit tests for the new function.Since this function controls critical connection behavior, it would be beneficial to add unit tests covering:
- Codespaces environment detection
- Colab context detection
- Default case (neither environment)
Would you like me to help create the unit test cases?
🧰 Tools
🪛 Ruff (0.8.2)
150-153: Return the condition
_get_context() == _COLABdirectlyReplace with
return _get_context() == _COLAB(SIM103)
|
LGTM |
| @@ -140,6 +141,18 @@ def _get_context(): | |||
| return _context | |||
|
|
|||
|
|
|||
| def _requires_http_polling(): | |||
| ### SSE is unreliable in GitHub Codespaces | |||
| if os.environ.get("CODESPACES", None) == "true": | |||
There was a problem hiding this comment.
how about:
| if os.environ.get("CODESPACES", None) == "true": | |
| if os.environ.get("CODESPACES", "false").lower() == "true": |
There was a problem hiding this comment.
true is precisely what we expect if we are in a GitHub Codepaces context. I think the above is sufficient
import os
import fiftyone as fo
os.environ["CODESPACES"] = "true"
session = fo.Session(remote=True)
assert "polling=true" in session.url |
What changes are proposed in this pull request?
SSE has been shown to be an unreliable
/eventsconnection in GitHub Codespaces. This change adds thepolling=trueURL search parameter to aSession.urlvalue automatically for a reliable HTTP polling connection in Codespaces. TheCODESPACES=trueenv var indicates a Codespaces context.Minor cleanup since Colab also requires HTTP polling.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
view_nameparameter for session initializationRefactor
The changes enhance the application's adaptability in various computing environments, particularly for Codespaces and Google Colab.