Add FiftyOneTorchDataset #5321
Add FiftyOneTorchDataset #5321
Conversation
…ther than view. Way faster.
WalkthroughThis pull request introduces a comprehensive set of tools and scripts for training machine learning models using the MNIST dataset with FiftyOne and PyTorch. The changes include a training script ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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 (8)
fiftyone/utils/torch.py (1)
1605-1605: Useisfor None checksThis line compares to
Noneusing==. For clarity and correctness, preferis None.- if self._samples == None: + if self._samples is None:🧰 Tools
🪛 Ruff (0.8.2)
1605-1605: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
docs/source/recipes/torch-dataset-examples/mnist_training.py (1)
12-39: [Function main: handle partial device availability?]Great job customizing device usage. Consider adding a fallback for cases where
"cuda:0"or the specified device is not available, e.g. defaulting to CPU.docs/source/recipes/torch-dataset-examples/utils.py (3)
1-1: Remove unused import if truly unnecessaryIt appears
import fiftyone as fois unused here. Removing it can improve clarity and avoid confusion.- import fiftyone as fo + # remove if truly not needed🧰 Tools
🪛 Ruff (0.8.2)
1-1:
fiftyoneimported but unusedRemove unused import:
fiftyone(F401)
131-131: Replace unnecessary ternary with direct comparisonUse a direct boolean expression to simplify logic.
- shuffle = True if split_tag == "train" else False + shuffle = (split_tag == "train")🧰 Tools
🪛 Ruff (0.8.2)
131-131: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
142-150: [setup_model: consider user flexibility]Some users might need a different backbone or weights. Possibly add a parameter to switch from ResNet18 to other architectures?
docs/source/recipes/torch-dataset-examples/simple_training_example.ipynb (1)
68-68: Typographical correction in markdown"traing" => "training" to maintain clarity in documentation.
- Now we will look at an actual traing script with `FiftyOneTorchDataset` + Now we will look at an actual training script with `FiftyOneTorchDataset`fiftyone/core/collections.py (2)
10973-10978: Consider adding type hints for better IDE supportThe method could benefit from type hints for the
get_itemparameter and return type to improve IDE support and code clarity.- def to_torch(self, get_item, **kwargs): + def to_torch(self, get_item: Callable[[Any], Any], **kwargs) -> "FiftyOneTorchDataset":
10973-10978: Consider adding validation for theget_itemparameterThe method should validate that
get_itemis a callable to provide a better error message.def to_torch(self, get_item, **kwargs): + if not callable(get_item): + raise ValueError( + "The 'get_item' parameter must be a callable that accepts a sample and " + "returns the corresponding tensor(s)" + ) from fiftyone.utils.torch import FiftyOneTorchDataset return FiftyOneTorchDataset(self, get_item, **kwargs)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/source/recipes/torch-dataset-examples/mnist_training.py(1 hunks)docs/source/recipes/torch-dataset-examples/simple_training_example.ipynb(1 hunks)docs/source/recipes/torch-dataset-examples/utils.py(1 hunks)fiftyone/core/collections.py(1 hunks)fiftyone/utils/torch.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/source/recipes/torch-dataset-examples/utils.py
1-1: fiftyone imported but unused
Remove unused import: fiftyone
(F401)
131-131: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
fiftyone/utils/torch.py
1605-1605: Comparison to None should be cond is None
Replace with cond is None
(E711)
🔇 Additional comments (25)
fiftyone/utils/torch.py (10)
13-15: [Imports look good]
No issues found with these imports.
32-34: [Imports look good]
No issues found with these imports.
41-41: [Distributed import looks good]
No issues found with this import.
1470-1551: [Comprehensive class docstring, few content suggestions]
The docstring is highly informative. However, consider briefly specifying the expected return type in get_item (e.g., dict, tensor, or custom object) to clarify type-checking needs.
1610-1614: [Worker init logic seems correct]
The worker_init approach is aligned with best practices for DataLoader workers. Ensure thorough testing in multi-worker scenarios to confirm memory usage remains stable.
1640-1645: [Good specialized logic for cached fields]
This dictionary-based approach for returning sample data is solid for decoupling data loading from the main process. Continue ensuring the data schema remains stable across training epochs.
2220-2244: [NumpySerializedList Implementation]
Serializing data to a numpy array of bytes is a clever approach. Be aware of potential overhead for extremely large datasets.
2255-2266: [TorchSerializedList Implementation]
Converting to torch tensors is convenient for consistency with PyTorch. Looks solid.
2268-2298: [Shared memory approach]
The design for distributing serialized data across local DDP ranks is well-thought-out. Ensure proper cleanup of shared memory resources after training completes to prevent memory leaks.
2300-2351: [DDP Communication Routines]
These distributed utilities (e.g., all_gather, local_scatter) are essential for multi-rank synchronization. Make sure you test corner cases with a single rank or large multi-GPU clusters.
docs/source/recipes/torch-dataset-examples/mnist_training.py (3)
12-39: [Potential File Save Race Condition]
When saving models (torch.save(...)), measuring the overhead or ensuring concurrency safety might be needed if multiple processes attempt to save.
62-83: [Function train_epoch: structured approach looks good]
Make sure large-scale loads into batch don’t cause memory fragmentation under certain expansions of dataset size or dynamic input shapes.
86-127: [Function validation: good usage of no_grad()]
This function properly detaches computations. Just be mindful that updates to dataset._dataset.select(...) inside your loop might cause overhead if repeated too frequently on large sets.
docs/source/recipes/torch-dataset-examples/utils.py (10)
17-37: [Function get_item_quickstart: correct bounding box transformation]
Looks good. Keep in mind that large bounding box lists might impact performance if done repeatedly.
39-59: [Function get_item_cached_quickstart: consistent approach with get_item_quickstart]
The code is consistent. Validate that the fields in sample_dict exactly match your bounding box transformations to avoid index or key errors.
61-63: [Collate function: flexible approach]
Returning tuples of (image, label) as separate items is typical. If you foresee multi-output tasks or dynamic batching, consider a more advanced approach.
65-75: [Simple DataLoader creation: good reference]
This sets the stage for an easy integration with FiftyOneTorchDataset. No issues noted.
77-84: [ids_in_dataloader: demonstration function]
This demonstration function is helpful for basic usage. Keep an eye out for large memory usage if the dataset is big.
86-101: [Mapping MNIST indexes to strings: straightforward & clear]
No issues found. This is a good reference for labeling in logs.
103-107: [convert_and_normalize: future deprecation]
The warning about ToTensor() is relevant. Keep in mind that a future replacement with v2.ToImage() and v2.ToDtype(...) might be necessary.
110-118: [mnist_get_item: looks correct]
The function is straightforward. If the label format changes, ensure this integer parsing remains valid.
120-139: [create_dataloaders: flexible for train/val/test splits]
Ensure you handle the scenario when one or more splits are empty or missing tags.
🧰 Tools
🪛 Ruff (0.8.2)
131-131: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
152-155: [setup_optim: standard setup]
This is a reasonable default. If hyperparameter tuning is frequent, consider making LR or L2 configurable at runtime.
fiftyone/core/collections.py (2)
10973-10978: LGTM: Clean implementation of the to_torch method
The implementation correctly delegates to FiftyOneTorchDataset while maintaining a clean interface. The docstring reference to the implementation class provides good discoverability.
10973-10978: Verify PyTorch dependency availability
Since this method integrates with PyTorch, we should verify that PyTorch is available when this method is called.
✅ Verification successful
PyTorch dependency is properly handled
The codebase already has proper PyTorch dependency verification through fou.ensure_torch() which is called before importing torch in the fiftyone/utils/torch.py module. This function verifies that both torch and torchvision are installed and importable. Since the to_torch() method imports from fiftyone.utils.torch, which handles the dependency check, no additional verification is needed in the method itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if torch is listed as a dependency
grep -r "torch" setup.py requirements*.txt
Length of output: 41
Script:
#!/bin/bash
# Search for torch-related imports and try-except blocks
rg "torch" -A 5 -B 5
Length of output: 82613
Script:
#!/bin/bash
# Search for optional dependencies setup
rg "extras_require" setup.py
Length of output: 28
Script:
#!/bin/bash
# Look for any torch-related error handling
ast-grep --pattern 'try {
$$$
import torch
$$$
} catch ($$$) {
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Look for torch import handling in the specific file
rg "torch" -B 5 -A 5 fiftyone/core/collections.py
# Look for any import error handling patterns
rg "ImportError" -B 5 -A 5 fiftyone/core/collections.py
Length of output: 3228
Script:
#!/bin/bash
# Check the torch utility module for any import error handling
rg "import.*torch" -B 5 -A 5 fiftyone/utils/torch.py
# Look for any torch-related imports in __init__.py files
rg "import.*torch" -B 5 -A 5 "**/__init__.py"
Length of output: 2442
Script:
#!/bin/bash
# Look for ensure_torch function implementation
rg "ensure_torch" -B 5 -A 5 fiftyone/utils/
# Look for any torch-related imports in utils directory
rg "import.*torch" -B 5 -A 5 fiftyone/utils/
Length of output: 15094
Script:
#!/bin/bash
# Look for ensure_torch implementation in core/utils.py
rg "def ensure_torch" -B 5 -A 10 fiftyone/core/utils.py
Length of output: 488
|
MAGE training successful. Very coarse test, but nice to see still. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/source/recipes/torch-dataset-examples/ddp_train.py (5)
39-45: Confirm dataset loading approachEach process is synchronously loading the same dataset locally, which can increase memory usage for large datasets. Confirm whether this is the intended design or if a shared or streamed approach would be more efficient.
81-81: Fix minor spelling in log message"New best lost achieved" appears to be a typo. Change "lost" to "loss" for clarity.
- print(f"New best lost achieved : {best_loss}. Saving model...") + print(f"New best loss achieved : {best_loss}. Saving model...")
86-90: Remove redundant.to(DEVICES[local_rank])callLine 89 and 90 both move the model to the same device. You can remove one to avoid duplication.
model = utils.setup_ddp_model( num_classes=num_classes, weights_path=f"{save_dir}/epoch_{best_epoch}.pt", ).to(DEVICES[local_rank]) - model.to(DEVICES[local_rank]) ddp_model = torch.nn.parallel.DistributedDataParallel( model, device_ids=[DEVICES[local_rank]] )
125-125: Correct the spelling of “cummulative_loss”Use “cumulative_loss” for clearer communication.
- cummulative_loss = 0 + cumulative_loss = 0And update its usage accordingly in lines 142 and 157 as well.
Also applies to: 142-142, 157-157
143-144: Refactor nested if statementsYou can combine these nested if statements for simpler logic, per the static analysis suggestion.
-if local_rank == 0: - if batch_num % 100 == 0: +if local_rank == 0 and batch_num % 100 == 0: pbar.set_description(...)Also applies to: 193-194
🧰 Tools
🪛 Ruff (0.8.2)
143-144: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
docs/source/recipes/torch-dataset-examples/utils.py (3)
1-1: Remove unused import [fiftyone].The module is never referenced in the code, so you can safely delete this import line.
- import fiftyone as fo🧰 Tools
🪛 Ruff (0.8.2)
1-1:
fiftyoneimported but unusedRemove unused import:
fiftyone(F401)
118-137: Function create_dataloaders: simplify conditional assignment to boolean.Line 129 uses
shuffle = True if split_tag == "train" else False. This can be simplified to:shuffle = (split_tag == "train")🧰 Tools
🪛 Ruff (0.8.2)
129-129: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
164-186: Function create_dataloaders_ddp: simplify conditional assignment to boolean.Line 175 uses
shuffle = True if split_tag == "train" else False. This can be simplified to:shuffle = (split_tag == "train")🧰 Tools
🪛 Ruff (0.8.2)
175-175: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/recipes/torch-dataset-examples/ddp_train.py(1 hunks)docs/source/recipes/torch-dataset-examples/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/source/recipes/torch-dataset-examples/ddp_train.py
143-144: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
193-194: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
docs/source/recipes/torch-dataset-examples/utils.py
1-1: fiftyone imported but unused
Remove unused import: fiftyone
(F401)
129-129: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
175-175: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
🔇 Additional comments (17)
docs/source/recipes/torch-dataset-examples/ddp_train.py (3)
14-30: Ensure environment variables are defined
The code relies on environment variables like WORLD_SIZE, LOCAL_WORLD_SIZE, and RANK. Consider adding error handling or clear messages if they are not set, to prevent runtime failures in distributed settings.
176-190: Evaluate performance impact of per-batch dataset updates
Calling dataset._dataset.select(batch["id"]).set_values(...) and samples.save() for each batch can be costly for large datasets. Consider batching result writing or deferring it until after validation to reduce overhead.
[performance]
201-207: Clarify execution instructions
The inline comment shows an example command for 6 processes (--nproc-per-node=6), but the example sets --devices 2 3 4 5 6 7, which are 6 GPUs. It would be helpful to clarify these arguments to ensure they match real-world usage scenarios (e.g., confirming GPU device IDs and process counts).
docs/source/recipes/torch-dataset-examples/utils.py (14)
2-2: Import usage .
from fiftyone.utils.torch import FiftyOneTorchDataset is used below, so this import is necessary.
4-15: Imports and augmentation pipeline .
No immediate concerns or suggestions. The augmentation pipeline is clearly defined and easy to follow.
17-37: Function get_item_quickstart .
The workflow to open the image, compute bounding boxes, and apply augmentations is clear and logical. Your handling of empty detections with a zero-tensor is appropriately done.
39-59: Function get_item_cached_quickstart .
Similar to get_item_quickstart, this approach for working with a cached sample dictionary is clear and logically consistent.
61-62: Function simple_collate_fn .
This is a valid approach that returns data in an easily unpackable tuple. No changes needed.
65-75: Function create_dataloader_simple .
DataLoader creation is straightforward, and the use of FiftyOneTorchDataset.worker_init is correctly applied. No issues found.
77-84: Function ids_in_dataloader .
Retrieving IDs from batches for verification is a helpful debugging utility. The assertion for batch size ensures the logic is working as intended.
87-101: Function mnist_index_to_label_string .
Provides a neat mapping from label indices to strings. No concerns here.
103-105: Transformation convert_and_normalize .
It neatly converts images to the proper format for PyTorch and normalizes pixel values. All good here.
108-116: Function mnist_get_item .
Processing MNIST samples into a dictionary is well-executed. The label extraction is direct and understandable.
140-148: Function setup_model .
Configuring a ResNet18 plus linear head is appropriately done. Loading weights conditionally is also clear.
150-153: Function setup_optim .
Simple, well-defined SGD optimizer creation. No issues spotted.
158-162: Function setup_ddp_model .
Applying convert_sync_batchnorm is the recommended practice for DDP. Steps are logical.
188-190: No-op main guard .
The if __name__ == "__main__": pass block can be helpful when multiprocessing in notebooks. It’s fine to keep as-is.
|
@mwoodson1 @manushreegangwar Can you please give this a review? |
| each of the processes running the main train script on the machine this | ||
| object is on. | ||
|
|
||
| Notes: |
There was a problem hiding this comment.
A lot of these general notes seem not directly related to this class itself but more on how it is used. IMO such points are more suited for tutorial / how-to docs than code documentation.
There was a problem hiding this comment.
I think that the most common usage should be covered in the class docs, but I do see your point. The recipes in this PR cover all of the notes mentioned.
However, I would like to have a place in the API documentation that lists all of these points, because the user shouldn't have to read a tutorial to understand the behavior of the object. See https://pytorch.org/docs/stable/data.html as an example (in fact, a lot of the points I mention are thoroughly discussed there). How would you suggest to do this?
There was a problem hiding this comment.
These are great notes but +1 to what Markus said.
There was a problem hiding this comment.
I think moving some of this into a user guide makes sense. You may want a product perspective on this though, they likely will have a good idea on what to do for this type of documentation.
There was a problem hiding this comment.
@brimoor any thoughts?
There was a problem hiding this comment.
I left some specific notes, mostly regarding documentation but here is some high level feedback
- The documentation for the FiftyOneTorchDataset class is much too long imo and should be relocated to a user guide or tutorial documentation
- A lot of the documentation seems distributed training focused. Some users may use this feature without training on multiple machines, especially our open source users. Documentation for both is important but should be separated.
- There are a lot of gotchas with this feature. The biggest one I see is only mentioned in the basic_example.ipynb example stating you need to call to_torch again after sampling, otherwise it's not suitable for multiprocessing. Put as many of these into warnings in the code itself or make sure to call all of them out in one place to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
fiftyone/utils/torch.py (6)
13-16: Remove unused import.The
warningsmodule is imported but never used in the code.-import warnings🧰 Tools
🪛 Ruff (0.8.2)
16-16:
warningsimported but unusedRemove unused import:
warnings(F401)
1609-1609: Use idiomatic Python None comparison.Replace
==withiswhen comparing withNonefor better readability and performance.- if self._samples == None: + if self._samples is None:🧰 Tools
🪛 Ruff (0.8.2)
1609-1609: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
1567-1567: Consider chunked loading for large datasets.Loading all sample IDs into memory at once via
_to_bytes_array(samples.values("id"))could be problematic for very large datasets. Consider implementing chunked loading or lazy evaluation.
1565-1566: Address TODO comment about file-based storage.The TODO comment suggests implementing file-based storage for large datasets. This would be a valuable optimization for handling large-scale data.
Would you like me to help implement a file-based storage solution or create a GitHub issue to track this enhancement?
1641-1641: Remove unnecessary pylint disable comment.The
# pylint: disable=unsubscriptable-objectcomment appears unnecessary as numpy arrays are subscriptable by default.- # pylint: disable=unsubscriptable-object sample = self._dataset[self.ids[index].decode()]
2234-2247: Replace print statements with proper logging.Using print statements for operational information is not ideal for production environments. Use the logger that's already defined at the top of the file.
- print( - "Serializing {} elements to byte tensors and concatenating them all ...".format( - len(lst) - ) - ) + logger.info( + "Serializing %d elements to byte tensors and concatenating them all ...", + len(lst) + ) self._lst = [_serialize(x) for x in lst] self._addr = np.asarray([len(x) for x in self._lst], dtype=np.int64) self._addr = np.cumsum(self._addr) self._lst = np.concatenate(self._lst) - print( - "Serialized dataset takes {:.2f} MiB".format( - len(self._lst) / 1024**2 - ) - ) + logger.info( + "Serialized dataset takes %.2f MiB", + len(self._lst) / 1024**2 + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/torch.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/utils/torch.py
16-16: warnings imported but unused
Remove unused import: warnings
(F401)
1609-1609: Comparison to None should be cond is None
Replace with cond is None
(E711)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (1)
fiftyone/utils/torch.py (1)
2304-2418: Well-implemented distributed training utilities!The distributed training utility functions are well-documented, handle edge cases appropriately, and follow best practices for distributed PyTorch training.
| each of the processes running the main train script on the machine this | ||
| object is on. | ||
|
|
||
| Notes: |
There was a problem hiding this comment.
These are great notes but +1 to what Markus said.
| if self._samples == None: | ||
| self._load_samples() |
There was a problem hiding this comment.
In general, properties should do trivial computations. Loading an FO dataset does not fit the bill IMO.
There was a problem hiding this comment.
This is a standard way of lazy loading attributes (which is needed here). The alternative with __getattr__ is far less clean. Do you know of other ways of doing this?
There was a problem hiding this comment.
I agree with Manushree here. While less clean, using _getattr__ is the more Pythonic way for this type of lazy loading scenario IMO. I don't think it's worth blocking this but if it can be switched in a reasonable amount of time I'd prefer it.
There was a problem hiding this comment.
+1 not a blocker. You can decide @jacobsela
There was a problem hiding this comment.
In that case, for the sake of getting this through in a timely manner I'll leave it as is for now.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fiftyone/utils/torch.py (3)
1471-1541: Consider moving implementation notes to tutorial docs.The class documentation is comprehensive but contains many implementation details that might be better suited for tutorial documentation, keeping the API documentation focused and concise.
1545-1547: Add type hints according to style guide.Add return type hints to methods and consider using more specific types from
typingmodule.- def __init__( - self, - samples: focol.SampleCollection, - get_item: Callable[fos.SampleView, Any], - cache_fields: list[str] = None, - local_process_group=None, + def __init__( + self, + samples: focol.SampleCollection, + get_item: Callable[[fos.SampleView], Any], + cache_fields: Optional[list[str]] = None, + local_process_group: Optional[Any] = None, + ) -> None:
2290-2292: Remove redundant rank check.The rank check is duplicated in the initialization method.
def __init__(self, lst: list, local_process_group): - if get_local_rank(local_process_group) == 0: - super().__init__(lst) - if get_local_rank(local_process_group) == 0: + if get_local_rank(local_process_group) == 0: + super().__init__(lst)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/torch.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/utils/torch.py
1559-1559: No explicit stacklevel keyword argument found
(B028)
1616-1616: Comparison to None should be cond is None
Replace with cond is None
(E711)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (4)
fiftyone/utils/torch.py (4)
13-16: LGTM! Clean import organization.The new imports are well-organized and properly grouped by functionality.
Also applies to: 33-35
2320-2434: LGTM! Well-implemented DDP utilities.The distributed training utility functions are well-documented and properly handle edge cases.
2281-2285:⚠️ Potential issueAdd bounds checking in getitem.
Add index validation to prevent potential IndexError.
def __getitem__(self, idx): + if not 0 <= idx < len(self): + raise IndexError("Index out of range") start_addr = 0 if idx == 0 else self._addr[idx - 1].item() end_addr = self._addr[idx].item()Likely invalid or redundant comment.
2250-2263: 🛠️ Refactor suggestionUse logging instead of print statements.
Replace print statements with proper logging for better debugging in production environments.
- print( - "Serializing {} elements to byte tensors and concatenating them all ...".format( - len(lst) - ) - ) + logger.info( + "Serializing %d elements to byte tensors and concatenating them all ...", + len(lst) + )Likely invalid or redundant comment.
| if self._samples == None: | ||
| self._load_samples() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix None comparison.
Replace == with is when comparing with None.
- if self._samples == None:
+ if self._samples is None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._samples == None: | |
| self._load_samples() | |
| if self._samples is None: | |
| self._load_samples() |
🧰 Tools
🪛 Ruff (0.8.2)
1616-1616: Comparison to None should be cond is None
Replace with cond is None
(E711)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
fiftyone/utils/torch.py (3)
1476-1479: Fix parameter description typos and grammar.The parameter descriptions contain minor issues:
get_item: Remove duplicate "from"cache_fields: Fix grammar in the description- Must be a serializable function. + Must be a serializable function that accepts a sample and returns the model input. - argument is passed, get_item should be from from a dict with keys and values + argument is passed, get_item should accept a dict with keys and values
1557-1561: Add stacklevel to warning.The warning about start method should include a stacklevel to point to the correct line in the user's code.
- warnings.warn( - f"Your start method is {start_method}. It is recommended to use 'spawn' or 'forkserver' with this class." - ) + warnings.warn( + f"Your start method is {start_method}. It is recommended to use 'spawn' or 'forkserver' with this class.", + stacklevel=2 + )🧰 Tools
🪛 Ruff (0.8.2)
1559-1559: No explicit
stacklevelkeyword argument found(B028)
1614-1618: Optimize lazy loading implementation.The samples property performs heavy computation by loading the dataset. Consider:
- Using
@functools.cached_propertyto cache the result- Adding a warning about the computational cost
- @property + @functools.cached_property def samples(self): - if self._samples == None: + if self._samples is None: + logger.warning("Loading dataset, this may take a moment...") self._load_samples() return self._samples🧰 Tools
🪛 Ruff (0.8.2)
1616-1616: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/torch.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/utils/torch.py
1559-1559: No explicit stacklevel keyword argument found
(B028)
1616-1616: Comparison to None should be cond is None
Replace with cond is None
(E711)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: build / changes
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (3)
fiftyone/utils/torch.py (3)
2320-2418: LGTM! Well-implemented distributed training utilities.The utility functions for distributed training are well-documented and handle edge cases properly, including:
- Proper fallbacks for non-distributed scenarios
- Clear documentation of return values
- Efficient caching of process groups
2290-2292: 🛠️ Refactor suggestionRemove redundant rank check.
The rank check is performed twice unnecessarily.
- if get_local_rank(local_process_group) == 0: - super().__init__(lst) - if get_local_rank(local_process_group) == 0: + if get_local_rank(local_process_group) == 0: + super().__init__(lst)Likely invalid or redundant comment.
2250-2263: 🛠️ Refactor suggestionReplace print statements with proper logging.
Use the logger instead of print statements for better debugging in production environments.
- print( - "Serializing {} elements to byte tensors and concatenating them all ...".format( - len(lst) - ) - ) + logger.info( + "Serializing %d elements to byte tensors and concatenating them all ...", + len(lst) + ) self._lst = [_serialize(x) for x in lst] self._addr = np.asarray([len(x) for x in self._lst], dtype=np.int64) self._addr = np.cumsum(self._addr) self._lst = np.concatenate(self._lst) - print( - "Serialized dataset takes {:.2f} MiB".format( - len(self._lst) / 1024**2 - ) - ) + logger.info( + "Serialized dataset takes %.2f MiB", + len(self._lst) / 1024**2 + )Likely invalid or redundant comment.
|
@mwoodson1 Thank you for the feedback. I've addressed some of the points and wanted to discuss some of the others.
I agree that it's cumbersome. I wrote a lengthier explanation on the comment above. The recipes added to this PR cover all of the points mentioned. I still think these points should be in the API docs. If the class itself isn't the right place we need to find a different solution.
DDP applies for both multi node and single-node, multi-gpu training. It's relevant almost universally for people that regularly train models. That said, I do agree that the added text is unappealing for new users. This goes back to the previous point, if not there, where should the DDP notes live?
I agree, it's a big issue. I've added warnings and early error throwing where possible. Unfortunately, a lot of the pitfalls occur outside of code we have purview over. In the example you gave (don't call |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
fiftyone/utils/torch.py (3)
1551-1553: Add stacklevel to warning.The warning about start method should include a stacklevel to help users identify the source of the warning.
- warnings.warn( - f"Your start method is {start_method}. It is recommended to use 'spawn' or 'forkserver' with this class." - ) + warnings.warn( + f"Your start method is {start_method}. It is recommended to use 'spawn' or 'forkserver' with this class.", + stacklevel=2 + )🧰 Tools
🪛 Ruff (0.8.2)
1551-1551: No explicit
stacklevelkeyword argument found(B028)
2277-2281: Replace print with logger.Use structured logging instead of print statements for better debugging in production environments.
- print( - "Serializing {} elements to byte tensors and concatenating them all ...".format( - len(lst) - ) - ) + logger.info( + "Serializing %d elements to byte tensors and concatenating them all ...", + len(lst) + )
2399-2424: Add timeout to all_gather operation.The all_gather operation could potentially hang indefinitely. Consider adding a timeout.
def all_gather(data, group=None): + timeout = 30.0 # seconds if get_world_size() == 1: return [data] if group is None: group = ( _get_global_gloo_group() ) # use CPU group by default, to reduce GPU RAM usage. world_size = dist.get_world_size(group) if world_size == 1: return [data] output = [None for _ in range(world_size)] - dist.all_gather_object(output, data, group=group) + dist.all_gather_object(output, data, group=group, timeout=timeout) return output
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
fiftyone/utils/torch.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
fiftyone/utils/torch.py
1551-1551: No explicit stacklevel keyword argument found
(B028)
1612-1612: Comparison to None should be cond is None
Replace with cond is None
(E711)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (2)
fiftyone/utils/torch.py (2)
2295-2299: 🛠️ Refactor suggestionAdd bounds checking in getitem.
The current implementation assumes valid indices but could raise cryptic errors for invalid ones.
def __getitem__(self, idx): + if not 0 <= idx < len(self): + raise IndexError("Index out of range") + start_addr = 0 if idx == 0 else self._addr[idx - 1].item() end_addr = self._addr[idx].item() bytes = memoryview(self._lst[start_addr:end_addr]) return pickle.loads(bytes)Likely invalid or redundant comment.
1611-1614: 🛠️ Refactor suggestionFix None comparison and optimize property implementation.
The property performs heavy computation by loading the dataset. Consider:
- Using
isinstead of==for None comparison- Using
@functools.cached_propertyto cache the result- @property - def samples(self): - if self._samples == None: - self._load_samples() - return self._samples + @functools.cached_property + def samples(self): + if self._samples is None: + self._load_samples() + return self._samplesLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
1612-1612: Comparison to
Noneshould becond is NoneReplace with
cond is None(E711)
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
docs/source/recipes/torch-dataset-examples/utils.py (3)
12-14: Consider making the augmentation pipeline configurable.The current augmentation pipeline uses a fixed center crop of 512x512, which might not be suitable for all input images and use cases. Consider making the crop size configurable or allowing users to pass custom augmentation pipelines.
-augmentations_quickstart = transforms.Compose( - [transforms.CenterCrop(512), transforms.ClampBoundingBoxes()] -) +def create_augmentation_pipeline(crop_size=512, additional_transforms=None): + transform_list = [ + transforms.CenterCrop(crop_size), + transforms.ClampBoundingBoxes() + ] + if additional_transforms: + transform_list.extend(additional_transforms) + return transforms.Compose(transform_list) + +augmentations_quickstart = create_augmentation_pipeline()
120-140: Simplify code and make split tags configurable.The function can be improved by:
- Making split tags configurable
- Simplifying the shuffle logic
- Adding type hints for better code clarity
def create_dataloaders( dataset, get_item, + split_tags=None, cache_fields=None, local_process_group=None, **kwargs ): - split_tags = ["train", "validation", "test"] + split_tags = split_tags or ["train", "validation", "test"] dataloaders = {} for split_tag in split_tags: split = dataset.match_tags(split_tag).to_torch( get_item, cache_fields=cache_fields, local_process_group=local_process_group, ) - shuffle = True if split_tag == "train" else False + shuffle = split_tag == "train" dataloader = torch.utils.data.DataLoader( split, shuffle=shuffle, worker_init_fn=FiftyOneTorchDataset.worker_init, **kwargs, ) dataloaders[split_tag] = dataloader return dataloaders🧰 Tools
🪛 Ruff (0.8.2)
131-131: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
190-192: Improve documentation for notebook compatibility.The comment should better explain why this code is necessary for notebook compatibility.
if __name__ == "__main__": - # this is just here to multiprocessing works when we call these functions in a notebook + # This empty main block is required for multiprocessing compatibility when + # running these functions in Jupyter notebooks. Without it, the worker + # processes would attempt to execute the notebook cells again. pass
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/recipes/torch-dataset-examples/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/source/recipes/torch-dataset-examples/utils.py
1-1: fiftyone imported but unused
Remove unused import: fiftyone
(F401)
131-131: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
177-177: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build / build
- GitHub Check: e2e / test-e2e
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
docs/source/recipes/torch-dataset-examples/utils.py (3)
12-14: Consider making crop size configurable.The
CenterCroptransform uses a fixed size of 512, which might not be suitable for all images or use cases. Consider making this configurable through a parameter.-augmentations_quickstart = transforms.Compose( - [transforms.CenterCrop(512), transforms.ClampBoundingBoxes()] -) +def create_augmentations(crop_size=512): + return transforms.Compose( + [transforms.CenterCrop(crop_size), transforms.ClampBoundingBoxes()] + ) + +augmentations_quickstart = create_augmentations()
133-133: Simplify shuffle logic.The conditional expression can be simplified.
- shuffle = True if split_tag == "train" else False + shuffle = split_tag == "train"🧰 Tools
🪛 Ruff (0.8.2)
133-133: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
179-179: Simplify shuffle logic.The conditional expression can be simplified.
- shuffle = True if split_tag == "train" else False + shuffle = split_tag == "train"🧰 Tools
🪛 Ruff (0.8.2)
179-179: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/source/recipes/torch-dataset-examples/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/source/recipes/torch-dataset-examples/utils.py
1-1: fiftyone imported but unused
Remove unused import: fiftyone
(F401)
133-133: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
179-179: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: build / build
- GitHub Check: build / changes
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build
🔇 Additional comments (6)
docs/source/recipes/torch-dataset-examples/utils.py (6)
17-39: Add error handling for image loading.The function should handle potential file I/O errors when opening images.
69-78: Make batch size and worker count configurable.The dataloader configuration has hardcoded values which limits flexibility.
81-87: Remove hardcoded batch size assertion.The assertion assumes a fixed batch size of 5, which doesn't align with making the batch size configurable.
112-119: Optimize MNIST image processing.Converting MNIST images (originally grayscale) to RGB is unnecessary and inefficient.
144-151: Add device placement and enhance model setup configurability.The model setup should handle device placement and allow for more configuration options.
168-190: Reduce code duplication in DDP dataloader creation.The
create_dataloaders_ddpfunction largely duplicates code fromcreate_dataloaders.🧰 Tools
🪛 Ruff (0.8.2)
179-179: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
docs/source/recipes/torch-dataset-examples/utils.py (3)
1-1: Remove unused import.The
fiftyoneimport is not directly used in this file.-import fiftyone as fo🧰 Tools
🪛 Ruff (0.8.2)
1-1:
fiftyoneimported but unusedRemove unused import:
fiftyone(F401)
12-14: Consider making augmentations configurable.The augmentations pipeline has hardcoded values. Consider making it more flexible by accepting parameters.
-augmentations_quickstart = transforms.Compose( - [transforms.CenterCrop(512), transforms.ClampBoundingBoxes()] -) +def create_augmentations(crop_size=512): + return transforms.Compose( + [transforms.CenterCrop(crop_size), transforms.ClampBoundingBoxes()] + ) + +augmentations_quickstart = create_augmentations()
137-137: Simplify boolean expression.The ternary operation can be simplified.
- shuffle = True if split_tag == "train" else False + shuffle = split_tag == "train"🧰 Tools
🪛 Ruff (0.8.2)
137-137: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
docs/source/recipes/torch-dataset-examples/ddp_train.py (1)
133-137: Simplify nested conditions and consider adding gradient clipping.The code can be improved in two ways:
- Combine nested if statements
- Add gradient clipping for training stability
- if local_rank == 0: - if batch_num % 100 == 0: + if local_rank == 0 and batch_num % 100 == 0: pbar.set_description( f"Average Train Loss = {cummulative_loss / (batch_num + 1):10f}" ) + # Add gradient clipping + torch.nn.utils.clip_grad_norm_(model.parameters(), max_norm=1.0)🧰 Tools
🪛 Ruff (0.8.2)
133-134: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/source/recipes/torch-dataset-examples/ddp_train.py(1 hunks)docs/source/recipes/torch-dataset-examples/utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
docs/source/recipes/torch-dataset-examples/ddp_train.py
133-134: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
183-184: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
docs/source/recipes/torch-dataset-examples/utils.py
1-1: fiftyone imported but unused
Remove unused import: fiftyone
(F401)
137-137: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
187-187: Remove unnecessary True if ... else False
Remove unnecessary True if ... else False
(SIM210)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build
🔇 Additional comments (7)
docs/source/recipes/torch-dataset-examples/utils.py (7)
17-39: Add error handling for image loading.The function should handle potential file I/O errors when opening images.
41-62: Reduce code duplication with get_item_quickstart.Consider extracting common functionality into a helper function.
69-78: Make batch size and worker count configurable.The dataloader configuration has hardcoded values which limits flexibility.
81-87: Remove hardcoded batch size assertion.The assertion assumes a fixed batch size of 5, which doesn't align with making the batch size configurable.
112-119: Optimize MNIST image processing.Converting MNIST images (originally grayscale) to RGB is unnecessary and inefficient.
148-155: Add device placement and enhance model setup configurability.The model setup should handle device placement and include error handling for weight loading.
172-197: Reduce code duplication in DDP dataloader creation.The function largely duplicates code from create_dataloaders.
🧰 Tools
🪛 Ruff (0.8.2)
187-187: Remove unnecessary
True if ... else FalseRemove unnecessary
True if ... else False(SIM210)
What changes are proposed in this pull request?
FiftyOneTorchDataset.to_torchmethod.How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
New
to_torchmethod allows for easy transfer to a fully functionaltorch.utils.data.Datasetobject.Please see recipes for examples and tutorials.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesfiftyone.utils.torchSummary by CodeRabbit
New Features
Bug Fixes
Documentation