-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add cudf snowflake example #505
Add cudf snowflake example #505
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@@ -0,0 +1,356 @@ | |||
{ |
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.
@@ -0,0 +1,356 @@ | |||
{ |
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.
Line #14. "token": get_login_token(),
You could replace this line with "token": pathlib.Path("/snowflake/session/token").read_text()
which would remove a little indirection while still being very readable.
Reply via ReviewNB
@@ -0,0 +1,356 @@ | |||
{ |
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.
I would make this note a warning admonition. We might also want to link to the issue so that readers can check on the progress.
Reply via ReviewNB
Looks like ReviewNB had an issue and created some duplicated comments. I'll clean those up now. |
We have an issue, I was trying to test the using
it's unclear to me why, I did turn of MFA just in case, I wonder if there is some token caching or something that might be causing a problem. I will nuke everything and start from scratch tomorrow and see if things work. It's unclear what is going on. |
55c4ae9
to
25b3161
Compare
I went through the deployment, and then connected and things worked. I'm not sure what was the hiccup yesterday but clean slate worked. I updated the example according to @jacobtomlinson suggestions, and rebased against main to update the branch. This should be ready for final review. |
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.
Awesome this looks great!
Closes #494
With the caveat of having to do this: