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

Add support for IPython magic #44

Merged
merged 14 commits into from
May 21, 2022
Merged

Add support for IPython magic #44

merged 14 commits into from
May 21, 2022

Conversation

rbpatt2019
Copy link
Collaborator

  • build(pyproject.toml): add ipython support
  • feat(pyprql/magic): adds ipython magic
  • feat(magic/prql.py): rough working functionality
  • feat(pyproject.toml): add duckdb-engine
  • fix(magic/prql.py): change default configurations
  • fix(magic/prql.py): fix printing of results
  • docs(*): document magic

rbpatt2019 added 7 commits May 6, 2022 06:40
Adds ipython-sql and ipython as dependencies. The goal is to wrap the former and pass it parsed prql.
This is a minimal working example. It will fail in many cases, namely when used as a line magic where arguments are passed.
Major changes still required. Currently assumes that line magic will only be used to set up connection strings and options. Additionally, cell magic only returns correct results if the results are passed to a local variable.
Allows magic to use duckdb connection strings.
Defaults autopandas to true and displayconn to false.
By providing an autoview parameter and manually specifying `_` as the return variable, we can ensure that results are always accessible and always printable.
Provides full Sphinx documentation for IPython/Jupyter Magic.
Applies encessary lints to the file, while also updating format to run on Python 3.10/
Bring pyproject and lock file into alignment and update cache number.
Use of a local pre-release version of poetry had caused divergent formats in the lockfiles. This should bring those back in line.
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Great job @rbpatt2019 !

In [7]: %config PRQLMagic.autoview = False
```

[ipysql]: https://github.com/catherinedevlin/ipython-sql
Copy link
Member

Choose a reason for hiding this comment

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

I actually hadn't seen this sort of link in markdown before — it's great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a big fan of them. They make the document so much more readable in plain text, and allow you to re-use hyperlinks.

1. displaycon is set to ``False``.

Additionally,
to working around some quirky behaviour,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
to working around some quirky behaviour,
to work around some quirky behaviour,

The variables local to the running IPython shell.
"""
# If cell is occupied, it must be parsed to SQL
if cell != "":
Copy link
Member

Choose a reason for hiding this comment

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

(nit: arguably more idiomatic, although your call)

Suggested change
if cell != "":
if cell:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not going to lie, some how in years of coding Python, I had never picked that empty strings were False-y! All for this change


# If cell is occupied and line is empty,
# we artificially populate line to ensure a return value.
if cell != "" and line == "":
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if cell != "" and line == "":
if cell and not line:

@@ -0,0 +1,184 @@
# Using the IPython and Jupyter Magic
Copy link
Member

Choose a reason for hiding this comment

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

Great docs! Maybe we can link to these from https://lang.prql.builders/ when they're live

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They'll go live (for the magic) once the PR is merged. The full docs live at https://pyprql.readthedocs.io/en/latest/# and are already live.

pyproject.toml Outdated
@@ -36,6 +36,10 @@ numpy = [
{version = "^1.22.3", python = "^3.8"}
]
prql-python = "^0"
ipython = "7.33.0"
Copy link
Member

Choose a reason for hiding this comment

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

This is quite restrictive — do we want ">=7"? Otherwise it's going to downgrade everyone who has ipython 8 installed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can't remember why I limited this initially...I think since we support python 3.7, and IPython > 8 requires python 3.8 or greater. But there are definitely better ways to specify this. I'll see what we can do

that's as simple as:

```shell
pip install PyPRQL
Copy link
Member

Choose a reason for hiding this comment

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

I had thought conventions were to have lowercase. Both seem to work though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Convention is lowercase. This is me just blindly accepting autocomplete :p

```

Now,
the output of the query is saved to `results`.
Copy link
Member

Choose a reason for hiding this comment

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

Very clearly explained, again great docs!

.github/workflows/cicd.yaml Show resolved Hide resolved
if "<<" in line:
print(local_ns[line.split()[0]])
else:
print(local_ns["_"])
Copy link
Member

Choose a reason for hiding this comment

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

V impressive how brief the code is here! Nice work.

@max-sixty
Copy link
Member

max-sixty commented May 18, 2022

FYI I get this error, but it might well be my dependencies. Could you confirm it works for you @rbpatt2019 ?

image

In text form:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Input In [2], in <cell line: 1>()
----> 1 get_ipython().run_line_magic('load_ext', 'pyprql.magic')

File /opt/homebrew/lib/python3.9/site-packages/IPython/core/interactiveshell.py:2304, in InteractiveShell.run_line_magic(self, magic_name, line, _stack_depth)
   2302     kwargs['local_ns'] = self.get_local_scope(stack_depth)
   2303 with self.builtin_trap:
-> 2304     result = fn(*args, **kwargs)
   2305 return result

File /opt/homebrew/lib/python3.9/site-packages/IPython/core/magics/extension.py:33, in ExtensionMagics.load_ext(self, module_str)
     31 if not module_str:
     32     raise UsageError('Missing module name.')
---> 33 res = self.shell.extension_manager.load_extension(module_str)
     35 if res == 'already loaded':
     36     print("The %s extension is already loaded. To reload it, use:" % module_str)

File /opt/homebrew/lib/python3.9/site-packages/IPython/core/extensions.py:76, in ExtensionManager.load_extension(self, module_str)
     69 """Load an IPython extension by its module name.
     70 
     71 Returns the string "already loaded" if the extension is already loaded,
     72 "no load function" if the module doesn't have a load_ipython_extension
     73 function, or None if it succeeded.
     74 """
     75 try:
---> 76     return self._load_extension(module_str)
     77 except ModuleNotFoundError:
     78     if module_str in BUILTINS_EXTS:

File /opt/homebrew/lib/python3.9/site-packages/IPython/core/extensions.py:93, in ExtensionManager._load_extension(self, module_str)
     91     with prepended_to_syspath(self.ipython_extension_dir):
     92         mod = import_module(module_str)
---> 93         if mod.__file__.startswith(self.ipython_extension_dir):
     94             print(("Loading extensions from {dir} is deprecated. "
     95                    "We recommend managing extensions like any "
     96                    "other Python packages, in site-packages.").format(
     97                   dir=compress_user(self.ipython_extension_dir)))
     98 mod = sys.modules[module_str]

AttributeError: 'NoneType' object has no attribute 'startswith'


## Implementation

This is a this wrapper around the fantastic
Copy link
Member

Choose a reason for hiding this comment

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

Small typo: "thin wrapper"


:::{Important}
This is one area where we differe from `IPython-sql`.
We only support parasing PRQL as a cell magic,
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "parsing PRQL"

:::{Important}
This is one area where we differe from `IPython-sql`.
We only support parasing PRQL as a cell magic,
not as a line magic.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused by this as in line 41 above you seem to have a line magic example, or is that passing a parameter to the prql magic which is then used in the cell magics further on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we do have a line magic, but it can only be used for connection strings and parameters. In the original Python-sql, line magic sql is valid, like this %sql select * from table. But they have quite an extensive parser to figure out whether the line magic is a connection string, flags, or sql. To the first order of approximation, they achieve this by checking to see if the line passed looks like SQL. So, if we wanted this functionality, we'd have to re-implement their parser, look for PRQL, parse the PRQL to SQL, re-assemble the line, then pass it to the magic. This seemed a bit of a faf, so for the momentI've limited the line magic to parameters and connections strings. I'll clarify in the docs!

Expands on our different uses of line and cell magic relative to IPython-SQL, and also corrects a few minor typos.
Empty strings are falsey while non empty strings are truth, so we can check them directly.
Allows those running more recent versions of Python to use more recent versions of IPython.
@rbpatt2019
Copy link
Collaborator Author

Also @max-sixty I can't reproduce the error using the most recent Jupyter notebook. I've updated the Python versions - let me know if the error still persists.

@qharlie
Copy link
Contributor

qharlie commented May 20, 2022

All of this is working great for me! I might take a stab at the line magic for inline queries after this is merged.

Also holy shiz everything is so fast!

If you connect to an existing SQL database,
then all the tables normally there will be accessible.

One thing we don't support that `IPython-SQL` does is passing PRQL directly to a line magic.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
One thing we don't support that `IPython-SQL` does is passing PRQL directly to a line magic.
One thing we don't support that `IPython-SQL` does is passing a PRQL query directly to a line magic.

and it may be added as a feature in a future release.

:::{Warning}
This is one area where we differe from `IPython-sql`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This is one area where we differe from `IPython-sql`.
This is one area where we differ from `IPython-sql`.

@max-sixty
Copy link
Member

Also @max-sixty I can't reproduce the error using the most recent Jupyter notebook. I've updated the Python versions - let me know if the error still persists.

I think I had installed it incorrectly or the wrong branch, I can't repro my error, at least on the current version. Mea culpa

@max-sixty
Copy link
Member

This is very cool indeed.

I def think we should merge! Let's also link to these docs from the Jupyter link here (and everywhere else we're listing general resources). I'm happy to do that.

One thing that would make it easier for newcomers, in the next version of the docs, is to have an example that's possible to follow along with, given that many of the primitives are new — e.g. the magics, the duckdb:///:memory:, the --persist, etc — even aside from PRQL itself. I realize writing docs is often less enjoyable so I'm happy to help here if needed.

Here's an example which is not a good example per se, but is possible to follow along with — we can change the df creation to be something more interesting / highlighs more of prql's features https://gist.github.com/max-sixty/dfb1521f326742358d5b0bdf4b1c9698

@rbpatt2019 rbpatt2019 merged commit c6784fe into main May 21, 2022
@rbpatt2019 rbpatt2019 deleted the fix043 branch May 21, 2022 06:19
@max-sixty
Copy link
Member

Congrats! I'm v excited about this!

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.

4 participants