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

Refine example notebooks #756

Merged
merged 19 commits into from
Oct 27, 2022
Merged

Refine example notebooks #756

merged 19 commits into from
Oct 27, 2022

Conversation

loomlike
Copy link
Collaborator

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
Fix local spark output file-format bug
b4f09d7 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Add dev dependencies. Add unit-test for local spark job launcher
283b7c8 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Fix local spark submission unused param error
d6c24bf 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@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 <42475935+loomlike@users.noreply.github.com>
@loomlike
Add dataset utilities and notebook path refactor. TODO: update refere…
8447f31 
…nce links

Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Add init.py to datasets module. Modify maybe_download to accept dir a…
ad4942c 
…s dst_path

Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Merge branch 'main' into jumin/example_notebook
d16d1e1
@loomlike
Add notebook test
b72ff3b 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
change notebook to use scrap flag and is_databricks
9ed4b96 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Fix databricks path
656d50d 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Fix unittest
06008ee 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@loomlike
Modify databricks notebook. Fix dbfs path errors in utils.
2a36d51 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@xiaoyongzhu
Copy link
Member

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
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.

feathr_project/feathr/datasets/nyc_taxi.py Outdated Show resolved Hide resolved
feathr_project/feathr/utils/job_utils.py Show resolved Hide resolved
feathr_project/feathr/utils/job_utils.py Show resolved Hide resolved
feathr_project/feathr/utils/platform.py Show resolved Hide resolved
@loomlike
Address review comments
4c020dc 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@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

@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
Merge branch 'main' into jumin/example_notebook
db25051
@loomlike
Revive feathr_config.yaml
bd9fdb3 
Signed-off-by: Jun Ki Min <42475935+loomlike@users.noreply.github.com>
@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

@blrchen blrchen blrchen approved these changes

@xiaoyongzhu xiaoyongzhu xiaoyongzhu 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