Skip to content

Refine example notebooks#756

Merged
blrchen merged 19 commits intofeathr-ai:mainfrom
loomlike:jumin/example_notebook
Oct 27, 2022
Merged

Refine example notebooks#756
blrchen merged 19 commits intofeathr-ai:mainfrom
loomlike:jumin/example_notebook

Conversation

@loomlike
Copy link
Collaborator

@loomlike loomlike commented Oct 18, 2022

Description

Resolves #749, #716, #707

How was this PR tested?

Added unit tests and notebook sample test (requires papermill and scrapbook packages -- added to setup.py's extra dependencies of "notebook" and "dev").

Does this PR introduce any user-facing changes?

  • No. You can skip the rest of this section.
  • Yes. Make sure to clarify your proposed changes.

loomlike added 13 commits October 6, 2022 21:28
@loomlike
Fix local spark output file-format bug
b4f09d7 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Add dev dependencies. Add unit-test for local spark job launcher
283b7c8 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Fix local spark submission unused param error
d6c24bf 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Merge branch 'main' into jumin/localspark_submission_fix
24aaaea
@loomlike
Refactor nyc_taxi example. TODO: update refs to the notebook
bb76c43 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Add dataset utilities and notebook path refactor. TODO: update refere…
8447f31 
…nce links

Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Add init.py to datasets module. Modify maybe_download to accept dir a…
ad4942c 
…s dst_path

Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Merge branch 'main' into jumin/example_notebook
d16d1e1
@loomlike
Add notebook test
b72ff3b 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
change notebook to use scrap flag and is_databricks
9ed4b96 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Fix databricks path
656d50d 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Fix unittest
06008ee 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Modify databricks notebook. Fix dbfs path errors in utils.
2a36d51 
Signed-off-by: Jun Ki Min <[email protected]>
xiaoyongzhu
xiaoyongzhu reviewed Oct 18, 2022
View reviewed changes
@xiaoyongzhu
Copy link
Member

xiaoyongzhu commented Oct 18, 2022

Thanks @loomlike ! feathr_project/feathrcli/data/feathr_user_workspace/features/non_agg_features.py for those python files, can we keep them? I think @jaymo001 is using those extensively

@xiaoyongzhu xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Oct 18, 2022
@jainr jainr removed request for hangfei and jaymo001 October 18, 2022 14:50
Yuqing-cat
Yuqing-cat reviewed Oct 19, 2022
View reviewed changes
Copy link
Collaborator

@Yuqing-cat Yuqing-cat left a comment

Choose a reason for hiding this comment

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

Amazing contribution Jun! I love the idea of this refine PR.
Doesn't have time to verify the deletion in feathrcli folder but the rest looks good.

@loomlike
Address review comments
4c020dc 
Signed-off-by: Jun Ki Min <[email protected]>
@loomlike
Copy link
Collaborator Author

loomlike commented Oct 20, 2022 •
edited
Loading

feathr_project/feathrcli/data/feathr_user_workspace/features/non_agg_features.py for those python files, can we keep them? I think @jaymo001 is using those extensively

@xiaoyongzhu Can you tell me which files & examples use them? Do you think we may move them under docs/samples instead?

@loomlike loomlike requested review from Yuqing-cat and xiaoyongzhu and removed request for Yuqing-cat and xiaoyongzhu October 24, 2022 14:54
@xiaoyongzhu
Copy link
Member

xiaoyongzhu commented Oct 24, 2022

@loomlike can we also keep this file? feathr_project/feathrcli/data/feathr_user_workspace/feathr_config.yaml

Also please fix the conflicts.

Otherwise I'm good, thanks for the PR!

@loomlike
Copy link
Collaborator Author

loomlike commented Oct 24, 2022 •
edited
Loading

Per discussion w/ @Yuqing-cat I'm making changes to pull_request_push_test.yml to install extra dependencies via pip install -e .[all]. Hope that fixes some of the test errors due to the missing packages. I'll add that in another PR.

@loomlike loomlike mentioned this pull request Oct 24, 2022
Closed
@loomlike
Copy link
Collaborator Author

loomlike commented Oct 24, 2022 •
edited
Loading

@loomlike can we also keep this file? feathr_project/feathrcli/data/feathr_user_workspace/feathr_config.yaml

Also please fix the conflicts.

@xiaoyongzhu done! Thank you for the review!

@blrchen blrchen merged commit 2f7e1fd into feathr-ai:main Oct 27, 2022
@blrchen blrchen mentioned this pull request Oct 31, 2022
Merged
2 tasks
blrchen added a commit that referenced this pull request Oct 31, 2022
@blrchen
Revert "Refine example notebooks (#756)"
a4a782e 
This reverts commit 2f7e1fd.
@blrchen blrchen mentioned this pull request Oct 31, 2022
Merged
blrchen added a commit that referenced this pull request Oct 31, 2022
@blrchen
Revert 756 (#798)
ff438f5 
* Revert "Refine example notebooks (#756)"

This reverts commit 2f7e1fd.

* Resolve conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yuqing-cat Yuqing-cat Yuqing-cat left review comments

@xiaoyongzhu xiaoyongzhu xiaoyongzhu approved these changes

@blrchen blrchen blrchen approved these changes

@jaymo001 jaymo001 Awaiting requested review from jaymo001

@hangfei hangfei Awaiting requested review from hangfei

@edwincheung edwincheung Awaiting requested review from edwincheung

@enya-yx enya-yx Awaiting requested review from enya-yx

@jainr jainr Awaiting requested review from jainr

@windoze windoze Awaiting requested review from windoze

Assignees

No one assigned

Labels

safe to test Tag to execute build pipeline for a PR from forked repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FR] Refine notebook examples

4 participants

@loomlike @xiaoyongzhu @blrchen @Yuqing-cat