-
Notifications
You must be signed in to change notification settings - Fork 260
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
Conversation
@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. |
@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 ( |
There was a problem hiding this 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.
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.