Expanding DataPipe to support DataFrames#71931
Expanding DataPipe to support DataFrames#71931VitalyFedyunin wants to merge 13 commits intogh/VitalyFedyunin/177/basefrom
Conversation
[ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
|
🔗 Helpful links
✅ No Failures (0 Pending)As of commit d33da8d (more details on the Dr. CI page): Expand to see more💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
[ghstack-poisoned]
ejguan
left a comment
There was a problem hiding this comment.
LGTM with a few questions.
| collate_fn: Callable = _utils.collate.default_collate, | ||
| conversion: Optional[ | ||
| Union[ | ||
| Callable[[Any], Any], |
There was a problem hiding this comment.
| Callable[[Any], Any], | |
| Callable[..., Any], |
|
|
||
|
|
||
| def _collate_helper(conversion, item): | ||
| # TODO(VitalyFedyunin): Verify that item is any sort of batch |
There was a problem hiding this comment.
Do we need to check the type of item here?
There was a problem hiding this comment.
And, do we want to extend to support list of dictionary?
| def _collate_helper(conversion, item): | ||
| # TODO(VitalyFedyunin): Verify that item is any sort of batch | ||
| if len(item.items) > 1: | ||
| print(item.items) |
There was a problem hiding this comment.
nit: removing print
And, I am curious where does the attribute items come from? What is the actual type of item?
|
Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as |
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
|
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Differential Revision: [D37500516](https://our.internmc.facebook.com/intern/diff/D37500516) [ghstack-poisoned]
|
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@pytorchbot merge -g |
|
@pytorchbot successfully started a merge job. Check the current status here |
|
Merge failed due to Refusing to merge as mandatory check(s) pull failed for rule superuser |
Differential Revision: [D37500516](https://our.internmc.facebook.com/intern/diff/D37500516) [ghstack-poisoned]
|
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Somehow internal tools lost approval. Need re-review. |
Differential Revision: [D37500516](https://our.internmc.facebook.com/intern/diff/D37500516) [ghstack-poisoned]
Differential Revision: [D37500516](https://our.internmc.facebook.com/intern/diff/D37500516) [ghstack-poisoned]
|
@pytorchbot merge -g |
|
@pytorchbot successfully started a merge job. Check the current status here |
|
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
|
@VitalyFedyunin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Hey @VitalyFedyunin. |
Summary: Pull Request resolved: #71931 Approved by: https://github.com/ejguan Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/bcab5257de4bbbbf307ee05c122f0b8c873af81a Original Phabricator Test Plan: Imported from OSS Reviewed By: malfet, mehtanirav Differential Revision: D37500516 Pulled By: VitalyFedyunin fbshipit-source-id: ce96469e92b90e1acf6cc764e707f34be2ba460e
|
|
||
| def __init__(self, callable, ctx=None, **kwargs): | ||
| if ctx is None: | ||
| self.ctx = {'operations': [], 'variables': []} |
There was a problem hiding this comment.
@VitalyFedyunin This seems to violate inheritance properties as base class Capture has more data. Usually you should call the parent's init before own init, but here it wouldn't really work. Many classes in this file have this issue.
Differential Revision: [D37500516](https://our.internmc.facebook.com/intern/diff/D37500516) Pull Request resolved: pytorch#71931 Approved by: https://github.com/ejguan
Stack from ghstack (oldest at bottom):
Differential Revision: D37500516