Add utils for supporting rerun panel plugin #4876
Conversation
WalkthroughThe changes in this pull request involve modifications to several files, primarily focusing on enhancing the functionality of schema handling and metadata management. Key updates include the addition of new functions for retrieving and checking embedded document types in Changes
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
|
sashankaryal
force-pushed
the
mvp/rerun-panel
branch
from
6a96d8d
to
8413c2b
Compare
sashankaryal
force-pushed
the
mvp/rerun-panel
branch
from
98e08e2
to
3b64f8a
Compare
sashankaryal
changed the title
![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 (7)
fiftyone/utils/rerun.py (2)
14-23: LGTM: Class definition and docstring are well-structured.The
RrdFileclass is properly defined with appropriate inheritance. The docstring is informative and explains the purpose and arguments of the class well.Consider adding a brief explanation of what an rrd file is in the class docstring, as it might not be immediately clear to all developers. For example:
"""Class for storing a rerun data (rrd) file and its associated metadata. An rrd file is a binary format used by Rerun to store and visualize data. Args: ... """
27-28: LGTM: Class attributes are well-defined, but consider improving version management.The
filepathandversionattributes are appropriately defined asStringFields. The default version for backwards compatibility is a good approach.Consider using a constant or configuration value for the default version instead of hardcoding it. This would make it easier to update the version across the codebase in the future. For example:
from fiftyone.constants import RERUN_DEFAULT_VERSION class RrdFile(DynamicEmbeddedDocument, fol._HasMedia): # ... version = fof.StringField(default=RERUN_DEFAULT_VERSION)This approach would centralize version management and make it easier to update when new Rerun versions are released.
app/packages/utilities/src/schema.ts (2)
55-74: LGTM! Consider a minor performance optimization.The implementation of
getFieldsWithEmbeddedDocTypeis correct and follows TypeScript best practices. It effectively traverses the schema recursively to find fields with the specifiedembeddedDocType.For a slight performance improvement when dealing with large schemas, consider using
Array.prototype.push.apply()to flatten the result array. Here's a suggested modification:export function getFieldsWithEmbeddedDocType( schema: Schema, embeddedDocType: string ): Field[] { const result: Field[] = []; function recurse(schema: Schema): Field[] { const localResult: Field[] = []; for (const field of Object.values(schema ?? {})) { if (field.embeddedDocType === embeddedDocType) { localResult.push(field); } if (field.fields) { Array.prototype.push.apply(localResult, recurse(field.fields)); } } return localResult; } Array.prototype.push.apply(result, recurse(schema)); return result; }This change reduces the number of array allocations and copies, which can be beneficial for performance in scenarios with deeply nested schemas.
76-93: LGTM! Consider a minor simplification.The implementation of
doesSchemaContainEmbeddedDocTypeis correct, efficient, and follows TypeScript best practices. It effectively traverses the schema recursively to check for the presence of the specifiedembeddedDocType.For improved readability, you can simplify the
recursefunction by removing the unnecessaryreturn falsestatement. Here's a suggested modification:export function doesSchemaContainEmbeddedDocType( schema: Schema, embeddedDocType: string ): boolean { function recurse(schema: Schema): boolean { return Object.values(schema ?? {}).some((field) => field.embeddedDocType === embeddedDocType || (field.fields && recurse(field.fields)) ); } return recurse(schema); }This change maintains the same functionality while making the code more concise and easier to read.
app/packages/state/src/utils.ts (1)
108-108: LGTM: Function signature updated correctlyThe
getStandardizedUrlsfunction signature has been updated to includeModalSample["urls"]as a possible type for theurlsparameter. This change increases the function's flexibility and is consistent with the import statement modification.Consider using a type alias for better readability:
type UrlsType = readonly { readonly field: string; readonly url: string }[] | { [field: string]: string } | ModalSample["urls"]; export const getStandardizedUrls = (urls: UrlsType) => { // ... function body ... };This change would make the function signature more concise and easier to maintain.
fiftyone/server/metadata.py (2)
37-37: LGTM: Addition of RrdFile to _ADDITIONAL_MEDIA_FIELDS is appropriate.The inclusion of
RrdFile: "filepath"in the_ADDITIONAL_MEDIA_FIELDSdictionary is consistent with the existing structure and supports the new rerun panel plugin functionality.Consider adding a brief comment above the dictionary to explain its purpose and the significance of the "filepath" value for each entry. This would enhance code readability and maintainability.
RrdFile support is not fully integrated
The
RrdFiletype is defined and imported in the codebase but is not utilized in any media type handling functions or conditionals. This suggests that additional integration steps are necessary to ensure full support forRrdFile.
fiftyone/utils/rerun.py:RrdFileclass is defined but not integrated into processing pipelines.fiftyone/server/metadata.py:RrdFileis imported and referenced in_ADDITIONAL_MEDIA_FIELDS, but no further usage is found in media handling functions.🔗 Analysis chain
Line range hint
27-37: Verify full integration of RrdFile support.The changes to support
RrdFilelook good, but it's important to ensure that this new type is fully integrated throughout the codebase.Please run the following script to check for any other necessary updates:
This will help identify any other areas of the code that might need updating to fully support the new
RrdFiletype.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential places where RrdFile support might be needed. # Search for functions or methods that handle different media types echo "Searching for media type handling:" rg --type python -n '(def|media_type|MediaType).*(\bimage\b|\bvideo\b|\bpoint_cloud\b|\bthree_d\b)' # Search for uses of _ADDITIONAL_MEDIA_FIELDS echo "Searching for uses of _ADDITIONAL_MEDIA_FIELDS:" rg --type python -n '_ADDITIONAL_MEDIA_FIELDS' # Search for other occurrences of RrdFile echo "Searching for other occurrences of RrdFile:" rg --type python -n 'RrdFile'Length of output: 552
Script:
#!/bin/bash # Description: Check for potential places where RrdFile support might be needed. # Search for functions or methods that handle different media types echo "Searching for media type handling:" rg -g "*.py" -n '(def|media_type|MediaType).*(\bimage\b|\bvideo\b|\bpoint_cloud\b|\bthree_d\b)' # Search for uses of _ADDITIONAL_MEDIA_FIELDS echo "Searching for uses of _ADDITIONAL_MEDIA_FIELDS:" rg -g "*.py" -n '_ADDITIONAL_MEDIA_FIELDS' # Search for other occurrences of RrdFile echo "Searching for other occurrences of RrdFile:" rg -g "*.py" -n 'RrdFile'Length of output: 9452
Script:
#!/bin/bash # Description: Ensure RrdFile is fully integrated across media handling functions. # Search for functions or methods that accept RrdFile as a parameter echo "Searching for functions accepting RrdFile as a parameter:" rg -g '*.py' -n 'def .*\(.*RrdFile.*\)' # Search for conditionals handling RrdFile in media processing echo "Searching for conditionals handling RrdFile:" rg -g '*.py' -n 'if .*RrdFile.*:' # Search for documentation or comments mentioning RrdFile integration echo "Searching for documentation or comments about RrdFile:" rg -g '*.py' -n '#.*RrdFile'Length of output: 451
Script:
#!/bin/bash # Description: Further verify RrdFile integration across the codebase. # Search for instantiations of RrdFile echo "Searching for instantiations of RrdFile:" rg -g '*.py' -n 'RrdFile\(' # Search for assignments or usages within data structures echo "Searching for assignments or usages of RrdFile within data structures:" rg -g '*.py' -n '[:=] .*RrdFile' # Search for RrdFile in class definitions or decorators echo "Searching for RrdFile in class definitions or decorators:" rg -g '*.py' -n '(class|@).*RrdFile.*' # Search for any other import patterns related to RrdFile echo "Searching for import patterns related to RrdFile:" rg -g '*.py' -n 'import .*RrdFile|from .* import .*RrdFile'Length of output: 879
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- app/packages/state/src/utils.ts (2 hunks)
- app/packages/utilities/src/schema.test.ts (7 hunks)
- app/packages/utilities/src/schema.ts (1 hunks)
- fiftyone/server/metadata.py (2 hunks)
- fiftyone/utils/rerun.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
app/packages/state/src/utils.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/utilities/src/schema.test.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/utilities/src/schema.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (13)
fiftyone/utils/rerun.py (2)
1-11: LGTM: File header and imports are well-structured.The file header includes a clear docstring explaining the purpose of the module, along with appropriate copyright information. The import statements are relevant to the functionality being implemented.
25-25: LGTM: _MEDIA_FIELD attribute is correctly defined.The
_MEDIA_FIELDclass attribute is properly set to "filepath", which aligns with thefol._HasMediainterface implementation.app/packages/state/src/utils.ts (1)
21-21: LGTM: Import statement updated correctlyThe import statement has been updated to include
ModalSamplefrom the "./recoil" module. This change is consistent with the modification in thegetStandardizedUrlsfunction.fiftyone/server/metadata.py (2)
27-27: LGTM: Import statement for RrdFile is appropriate.The import of
RrdFilefromfiftyone.utils.rerunaligns with the PR objective of supporting the rerun panel plugin. The specific import is well-placed and consistent with the existing code style.
Line range hint
1-524: Overall assessment: Changes look good, with minor suggestions for improvement.The changes to support the rerun panel plugin through the addition of
RrdFileare minimal, focused, and well-integrated with the existing code structure. The modifications align with the PR objectives and maintain good code quality.Summary of review comments:
- The import statement for
RrdFileis appropriate and well-placed.- The addition to
_ADDITIONAL_MEDIA_FIELDSis consistent and supports the new functionality.- A suggestion was made to add a brief comment explaining the purpose of
_ADDITIONAL_MEDIA_FIELDS.- A verification step was requested to ensure full integration of
RrdFilesupport throughout the codebase.These changes appear to be a solid foundation for supporting the rerun panel plugin. Once the verification step is completed and any necessary adjustments are made, this PR should be ready for merging.
app/packages/utilities/src/schema.test.ts (8)
22-24: Updates to 'embedded' schema object are correctThe
nameandpathproperties for theembeddedschema object are correctly updated to"embedded", ensuring consistency with the object key and improving clarity.
33-33: Updated path reflects correct hierarchy for nested fieldThe
pathproperty for thefieldwithinembeddedis correctly updated to"embedded.field", accurately representing its nested position in the schema hierarchy.
45-47: Updates to 'embeddedWithDbFields' schema object are correctThe
nameandpathproperties for theembeddedWithDbFieldsschema object are correctly set to"embeddedWithDbFields", maintaining consistency with the object key and enhancing readability.
57-57: Updated path reflects correct hierarchy for 'sample_id' fieldThe
pathproperty forsample_idwithinembeddedWithDbFieldsis correctly updated to"embeddedWithDbFields.sample_id", accurately reflecting its nested structure in the schema.
Line range hint
65-80: Tests for 'getCls' function are comprehensive and correctThe added test cases for the
getClsfunction effectively cover:
- Retrieving class names for top-level fields.
- Retrieving class names for embedded fields.
- Handling of missing field paths by returning
undefined.This ensures that the
getClsfunction behaves as expected across different scenarios.
Line range hint
82-110: Tests for 'getFieldInfo' function are thorough and accurateThe test suite for the
getFieldInfofunction includes:
- Retrieving field information for top-level fields.
- Retrieving field information for embedded fields.
- Verifying the
pathWithDbFieldproperty.- Handling of missing field paths by returning
undefined.These tests enhance confidence in the correctness of
getFieldInfoacross various use cases.
113-145: Tests for 'getFieldsWithEmbeddedDocType' function are well-designedThe new test cases for
getFieldsWithEmbeddedDocTypeappropriately verify:
- Retrieval of fields with a specific
embeddedDocTypeat the top level.- Retrieval of fields with a specific
embeddedDocTypewithin nested fields.- Correct handling when the specified
embeddedDocTypedoes not exist.- Correct handling when the schema is empty.
This comprehensive testing ensures reliable functionality of the
getFieldsWithEmbeddedDocTypemethod.
146-176: Tests for 'doesSchemaContainEmbeddedDocType' function are comprehensiveThe added tests for
doesSchemaContainEmbeddedDocTypefunction effectively check:
- Detection of
embeddedDocTypeat the top level.- Detection of
embeddedDocTypewithin nested fields.- Correct return value when the
embeddedDocTypedoes not exist.- Correct handling of an empty schema.
These tests confirm that the function accurately determines the presence of an
embeddedDocTypein various scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
fiftyone/utils/rerun.py (2)
14-20: Enhance docstring format for better readability.While the docstring provides clear information about the class and its arguments, consider reformatting it to follow the Google Python Style Guide for improved readability and consistency with other parts of the project.
Here's a suggested improvement:
class RrdFile(DynamicEmbeddedDocument, fol._HasMedia): """Class for storing a rerun data (rrd) file and its associated metadata. Args: filepath (str, optional): The path to the rrd file. Defaults to None. version (str, optional): The version of the rrd file. Defaults to None. """
14-25: Consider implementing required methods and providing usage context.The
RrdFileclass provides a good foundation for storing rrd file information. However, there are a couple of points to consider:
Since the class implements the
fol._HasMediainterface, ensure that all required methods from this interface are implemented. For example, you might need to override methods likeget_media_path()orset_media().To align with the PR objectives of supporting the rerun panel plugin, it would be helpful to provide more context on how this class will be used. Consider adding comments or documentation explaining its role in the larger system.
Example of implementing a required method:
def get_media_path(self): return self.filepathAdding a class-level comment for context:
class RrdFile(DynamicEmbeddedDocument, fol._HasMedia): """Class for storing a rerun data (rrd) file and its associated metadata. This class is used as part of the rerun panel plugin to manage and track rrd files within the FiftyOne ecosystem. ... """
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- fiftyone/utils/rerun.py (1 hunks)
🔇 Additional comments (1)
fiftyone/utils/rerun.py (1)
1-11: LGTM: File header and imports are well-structured.The file header includes a clear docstring explaining the purpose of the module, appropriate copyright information, and relevant imports for the functionality being implemented.
* add dynamic embedded document field type for rrd files * add schema utils to work with embedded doc type fields * add versioned renderer with url+metadata support * remove explicit version
* add dynamic embedded document field type for rrd files * add schema utils to work with embedded doc type fields * add versioned renderer with url+metadata support * remove explicit version
This PR introduces data model to support rerun panel plugin.
Summary by CodeRabbit
New Features
getFieldsWithEmbeddedDocTypeanddoesSchemaContainEmbeddedDocType.RrdFile, allowing for better metadata management and retrieval of file paths.RrdFileclass.Bug Fixes
Tests