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

[Azure DataStore] Handle storage options as secrets #1206

Merged

Conversation

hayesgb
Copy link
Contributor

@hayesgb hayesgb commented Aug 15, 2021

This add the ability to pass standard dictionary keys from fsspec's storage_options parameter into mlrun.run.get_dataitem() as secrets.

Enabling this will more easily allow users engaging in exploratory analysis to leverage the mlrun api to fetch data_items from Azure by enabling the following

storage_options={'account_name': "<NAME>", 'credential': <CREDENTIAL>}
df = mlrun.run.get_dataitem("az://CONTAINER/myfile.parquet", secrets=storage_options).as_df()

Copy link
Contributor

@Hedingber Hedingber left a comment

Choose a reason for hiding this comment

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

@hayesgb Looks good to me, just note you have some conflict

@Hedingber Hedingber changed the title Handle storage options as secrets [Azure DataStore] Handle storage options as secrets Aug 15, 2021
@hayesgb
Copy link
Contributor Author

hayesgb commented Aug 15, 2021

Fixed merge conflict. Thanks!

@Hedingber
Copy link
Contributor

@hayesgb I now noticed that the test file you added is almost an exact duplicate of test_azure_blob.py
I feel like we can merge them without too much effort
Looks like you'll need to make the verify_auth... method to behave a bit different whether it's env vars or storage options (can be determined by whether one of the params starts with AZURE_)
And in the tests simply always pass storage options to secrets, just in the case of env vars, it will be None
WDYT ?

@hayesgb
Copy link
Contributor Author

hayesgb commented Aug 17, 2021

@Hedingber -- I considered combining the tests, but was concerned about the interpretability. The two approaches serve entirely different use-cases, and I was worried that if the tests were blended (storing secrets or retrieving environmental variables vs user-passed credentials, which will most likely be done during exploratory analysis) it would lead to confusion and create potential maintainability issues. Thoughts?

@Hedingber
Copy link
Contributor

@hayesgb I see your point but I feel like leaving the tests duplicated will hurt maintainability more 😬 , @theSaarco WDYT ?

@hayesgb
Copy link
Contributor Author

hayesgb commented Aug 17, 2021

@Hedingber -- No worries. I consolidated the tests into a single file and updated the PR. LMKWYT.

@hayesgb hayesgb requested a review from Hedingber August 17, 2021 23:22
@Hedingber Hedingber merged commit 936a829 into mlrun:development Aug 18, 2021
@Hedingber
Copy link
Contributor

Hedingber commented Aug 18, 2021

@hayesgb Looks great!
merged
Big appreciation for the detailed comment in the verify auth method 👏 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants