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.

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

Added local_folder param to get_result_df (#394) #400

Merged
merged 5 commits into from
Jul 6, 2022

Conversation

shivamsanju
Copy link
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 xiaoyongzhu added the safe to test Tag to execute build pipeline for a PR from forked repo label Jun 27, 2022
@xiaoyongzhu xiaoyongzhu linked an issue Jun 27, 2022 that may be closed by this pull request
@xiaoyongzhu
Copy link
Member

xiaoyongzhu commented Jun 27, 2022

@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
Copy link
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
Copy link
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
Copy link
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 xiaoyongzhu requested a review from windoze June 27, 2022 14:54
@shivamsanju shivamsanju requested a review from hangfei July 2, 2022 11:14
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.

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

@xiaoyongzhu xiaoyongzhu merged commit ff2759e into feathr-ai:main Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test Tag to execute build pipeline for a PR from forked repo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add a local folder option in get_result_df
4 participants