-
Notifications
You must be signed in to change notification settings - Fork 110
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
provide some IPython implementations #956
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #956 +/- ##
==========================================
- Coverage 98.59% 98.14% -0.45%
==========================================
Files 50 50
Lines 8962 9013 +51
Branches 1598 1607 +9
==========================================
+ Hits 8836 8846 +10
- Misses 49 90 +41
Partials 77 77
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
def _ipython_key_completions_(self) -> List[str]: | ||
"""Provide custom key completions for the DataFrame in IPython or Jupyter Notebook. | ||
|
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.
a simple unit test can be added here
f"There are {count} rows. Showing only {rows_limit}. Change DataFrame.__rows_limit value to display more rows" | ||
) |
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.
f"There are {count} rows. Showing only {rows_limit}. Change DataFrame.__rows_limit value to display more rows" | |
) | |
f"There are {count} rows. Showing only {rows_limit}. Change DataFrame.__rows value to display more rows" | |
) |
the class variable is called __rows
import IPython | ||
import pandas as pd |
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.
Can you elaborate on when this function is called and what happens when pandas is not available
For a safe way to import pandas, you can follow
from snowflake.connector.options import installed_pandas, pandas |
we should have dependencies well documented when users want to use IPython magics. My guess is that IPython is always installed when running inside any IPython notebook.
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.
Yes IPython will always be available in jupyter. For the pandas... yes we should check for that. I see know that there is an existing mechanism for checking pandas presence. I think I can use that and if not just use df.show
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 guess this function is only called by IPython notebook
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.
yes, it is a ipython method: https://ipython.readthedocs.io/en/stable/config/integrating.html
# class property to control the number of rows to diplay | ||
__rows = 50 |
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.
Is this naming convention followed by ipython variables? I think there should be a better way to set this property because users should not be accessing variables that start with _
.
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.
No, I was just thinking on a way to expose a mechanism for setting the number of rows to display by default. Maybe a more explicit value in the session object for example will be better.
def register_sql_magic(): | ||
try: |
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.
can you describe what is the purpose of this function or point me to some documentation I can follow
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.
the purpose of this function is that when an import is done it will run init.py and call register_sql_magic. This will function will try to gather the ipython environment and if found it will register the sql magic
except Exception as ex: | ||
return str(ex) |
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.
how is the exception displayed on the client side. Is it just a cell with error message?
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 will just displayed in the cell output. Would you like to edit the traceback?
user_ns = IPython.get_ipython().user_ns | ||
if "session" in user_ns: | ||
session = user_ns["session"] |
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.
how does user_ns get populated with objects? how do we know that snowflake.snowpark.Session
will always be mapped to "session"
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.
We don't know before hand, that why we check and if it is not there we notify the user. We can iterate thru all the objects until we find a session object as well.
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.
we would either need to document that session object must be named session
and no other object must be named session, else these changes won't work
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
Fixes SNOW-871864: Better IPython integration #955
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This PR extends some classes adding IPython extensions