FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Added local_folder param to get_result_df (#394) by shivamsanju · Pull Request #400 · feathr-ai/feathr · GitHub

/ feathr Public

Added local_folder param to get_result_df (#394) - #400

Merged
xiaoyongzhu merged 5 commits into
feathr-ai:mainfrom
shivamsanju:main
Jul 6, 2022
ConversationCommits (5)ChecksFiles changed
Merged

Added local_folder param to get_result_df (#394)#400
xiaoyongzhu merged 5 commits into
feathr-ai:mainfrom
shivamsanju:main

Conversation

shivamsanju commented Jun 26, 2022

Copy link
Copy Markdown
Contributor

This PR addresses issue #394 :
Previously users were not getting an option to download the result dataframe on the local directory because a temporary directory was getting created which was cleaned up after reading the dataframe.

Changes:
Now an optional parameter called local_folder is added where users can pass the local directory name where they want to download the result. If the user does not pass this parameter, the function will create a temporary directory and clean it after reading the dataframe.

xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Jun 27, 2022
xiaoyongzhu linked an issue Jun 27, 2022 that may be closed by this pull request
add a local folder option in get_result_df #394
Closed

xiaoyongzhu commented Jun 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

@shivamsanju thanks for the contribution! Overall this looks good to me. I put a few comments inline.

Can you also merge main branch back? There might be some typos that we just fixed in the main branch which might be impactful for this PR.

xiaoyongzhu reviewed Jun 27, 2022
View reviewed changes
Comment thread feathr_project/feathr/utils/job_utils.py Outdated

xiaoyongzhu commented Jun 27, 2022

Copy link
Copy Markdown
Member

@shivamsanju can you merge the main branch of Feathr into this PR? Seems like those are the root cause for those test failures

shivamsanju commented Jun 27, 2022

Copy link
Copy Markdown
Contributor Author

@shivamsanju can you merge the main branch of Feathr into this PR? Seems like those are the root cause for those test failures

Hi @xiaoyongzhu, I have already merged the main branch of linkedin/feathr to this PR (commit b2ee907). It says already up to date when I do a git pull from linkedin/feathr.

xiaoyongzhu reviewed Jun 27, 2022
View reviewed changes
Comment thread feathr_project/feathr/utils/job_utils.py Outdated

xiaoyongzhu commented Jun 27, 2022

Copy link
Copy Markdown
Member

@shivamsanju can you merge the main branch of Feathr into this PR? Seems like those are the root cause for those test failures

Hi @xiaoyongzhu, I have already merged the main branch of linkedin/feathr to this PR (commit b2ee907). It says already up to date when I do a git pull from linkedin/feathr.

Ah OK so it might be a small typo that I've pointed out (feathr_spark_laucher to feathr_spark_launcher)

xiaoyongzhu requested a review from windoze June 27, 2022 14:54
shivamsanju requested a review from hangfei July 2, 2022 11:14
Yuqing-cat approved these changes Jul 6, 2022
View reviewed changes

Yuqing-cat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It would be better to have a log to record the local folder path if not None.

xiaoyongzhu merged commit ff2759e into feathr-ai:main Jul 6, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

xiaoyongzhu xiaoyongzhu approved these changes
Yuqing-cat Yuqing-cat approved these changes
jaymo001 Awaiting requested review from jaymo001
blrchen Awaiting requested review from blrchen
jainr Awaiting requested review from jainr
windoze Awaiting requested review from windoze
hangfei Awaiting requested review from hangfei

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.

add a local folder option in get_result_df

4 participants

Back | FazBrowse Home | New Git URL