Add FrameNumberField support for lightning queries #5093
Conversation
benjaminpkane
added
app
WalkthroughThis pull request introduces a new mapping for Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (1)
tests/unittests/lightning_tests.py (1)
291-295: Consider adding edge cases for frame numbersWhile the basic test cases are good, consider adding edge cases such as:
- Zero frame numbers
- Negative frame numbers (if supported)
- Large frame numbers
keys = _add_samples( dataset, - dict(ints=1, frame_numbers=1, frame_supports=[1, 1]), - dict(ints=2, frame_numbers=2, frame_supports=[2, 2]), + dict(ints=1, frame_numbers=0, frame_supports=[1, 1]), + dict(ints=2, frame_numbers=999999, frame_supports=[2, 2]), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
fiftyone/server/lightning.py(1 hunks)tests/unittests/lightning_tests.py(1 hunks)
🔇 Additional comments (4)
tests/unittests/lightning_tests.py (4)
285-290: LGTM: Test class setup follows established patterns
The test class correctly extends unittest.IsolatedAsyncioTestCase and properly sets up the required fields using appropriate field types.
297-308: LGTM: GraphQL query is well-structured
The query correctly uses the IntLightningResult type and requests all necessary fields (max, min, path).
309-314: LGTM: Test execution properly includes all integer field types
The test correctly includes all three field types (FrameNumberField, FrameSupportField, IntField) in the execution.
316-374: Verify comprehensive path coverage
The assertions cover all possible paths for integer fields in the dataset schema. However, let's verify that no paths are missing.
✅ Verification successful
Comprehensive path coverage verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all possible integer field paths are covered in the test assertions
# Search for all field path patterns in the codebase
rg -U "path.*frame_(numbers|supports)" | grep -v "test_ints"
# Search for any additional integer field types that might need testing
ast-grep --pattern 'class $name extends IntField {
$$$
}'
Length of output: 1544
* add missing frame number and frame support field support for lightning queries * rm frame support, not applicable
What changes are proposed in this pull request?
Adds missing FrameNumberField support for lightning queries. With QP enabled, the below field will error when expanding it in the sidebar
How is this patch tested? If it is not, please explain why.
Unit tests
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Tests