Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add pass-in value of edit fields for petastorm datamodule in py… #3651

Merged
merged 6 commits into from Aug 17, 2022
Merged

feat: add pass-in value of edit fields for petastorm datamodule in py… #3651

merged 6 commits into from Aug 17, 2022

Conversation

serena-ruan
Copy link
Contributor

@serena-ruan serena-ruan commented Aug 15, 2022 •
edited

…torch_lightning

Checklist before submitting

  • Did you read the contributor guide?
  • Did you update the docs?
  • Did you write any tests to validate this change? Not yet, and I'm open to suggestions.
  • Did you update the CHANGELOG, if this change affects users? Yes

Description

Fixes # (issue).
#3650

Review process to land

  1. All tests and other checks must succeed.
  2. At least one member of the technical steering committee must review and approve.
  3. If any member of the technical steering committee requests changes, they must be addressed.

@serena-ruan
feat: add pass-in value of edit fields for petastorm datamodule in py…
f610e82 
…torch_lightning

Signed-off-by: Xinyue Ruan <serena.rxy@gmail.com>
@serena-ruan
update CHANGELOG
e1b1c6e 
Signed-off-by: Xinyue Ruan <serena.rxy@gmail.com>
@serena-ruan
Copy link
Contributor Author

If this is fine, we could also support removed_fields and selected_fields 😃

@chongxiaoc
Copy link
Collaborator

LGTM. Let's see how CI goes.

chongxiaoc
chongxiaoc reviewed Aug 15, 2022
View reviewed changes
horovod/spark/lightning/datamodule.py Outdated
if self.transformation is None and self.transformation_edit_fields is None:
transform_spec = None
else:
transform_spec = TransformSpec(func=self.transformation, edit_fields=self.transformation_edit_fields)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment of linking TransformSepc constructor usage from Petastorm?
https://github.com/uber/petastorm/blob/3f248003221a648261a36189c95c8705f6ef34ad/petastorm/transform.py#L27

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'll also add param for removed_fields and selected_fields then :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since only one of removed_fields and selected_fields can be specified, I'll go with removed_fields as if you don't return a certain field, it echoes errors, but keeping extra fields should be fine.

chongxiaoc
chongxiaoc reviewed Aug 15, 2022
View reviewed changes
Copy link
Collaborator

@chongxiaoc chongxiaoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add a link from petastorm for constructor of transformspec.

@github-actions
Copy link

github-actions bot commented Aug 16, 2022 •
edited

Unit Test Results

  1 117 files   -    114    1 117 suites   - 114   11h 19m 49s ⏱️ - 39m 11s
     816 tests ±       0       765 ✔️ ±       0       51 💤 ±    0  0 ❌ ±0 
21 811 runs   - 2 448  15 567 ✔️  - 1 630  6 244 💤  - 818  0 ❌ ±0 

Results for commit 5f66963. ± Comparison against base commit 867741e.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Aug 16, 2022 •
edited

Unit Test Results (with flaky tests)

  1 260 files   -    174    1 260 suites   - 174   11h 49m 29s ⏱️ - 1h 8m 3s
     816 tests ±       0       765 ✔️ +       2       51 💤 ±       0  0 ❌  - 2 
24 591 runs   - 3 224  17 195 ✔️  - 1 978  7 396 💤  - 1 244  0 ❌  - 2 

Results for commit 5f66963. ± Comparison against base commit 867741e.

♻️ This comment has been updated with latest results.

@serena-ruan
add removed_fields and format with autopep8
68a2061 
Signed-off-by: Xinyue Ruan <serena.rxy@gmail.com>
@serena-ruan
fix typo
99b2ffb 
Signed-off-by: Xinyue Ruan <serena.rxy@gmail.com>
@serena-ruan
remove extra variable
5f66963 
Signed-off-by: Xinyue Ruan <serena.rxy@gmail.com>
@maxhgerlach maxhgerlach linked an issue Aug 17, 2022 that may be closed by this pull request
Can't pass-in edit_fields for TransformSpec in pytorch_lightning #3650
Closed
chongxiaoc
chongxiaoc approved these changes Aug 17, 2022
View reviewed changes
Copy link
Collaborator

@chongxiaoc chongxiaoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. waiting for CI to complete and merge. Thanks!

@chongxiaoc chongxiaoc merged commit da4bef4 into horovod:master Aug 17, 2022
@serena-ruan serena-ruan deleted the serena/fixTransformSpec branch August 18, 2022 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chongxiaoc chongxiaoc chongxiaoc approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Can't pass-in edit_fields for TransformSpec in pytorch_lightning
2 participants
@serena-ruan @chongxiaoc