Feature/improve cli #231

rajagurunath · 2021-08-29T14:27:48Z

Improved dask-sql CLI:

Autocompletion (for metacommands)
AutoSuggestion (from the previous history)
Added progress bar (Not sure if this is really needed)
Added metacommands (somewhat similar to pgcli)

improves CLI as mentioned in one of the issues here: #57

Increase Speed (dask-contrib#29)

Author: rajagurunath <[email protected]> Date: Mon May 24 02:37:40 2021 +0530

1. SHOW MODELS 2. DESCRIBE MODEL 3. EXPORT MODEL

Feature/export model

Fetch upstream changes to resolve conflicts

Fix a failing build, as ciso8601 is currently not pip-installable (dask-contrib#192)

…main

…ls-braun-main

1. Autocompletion 2. AutoSuggestion 3. Added progressBar 4. Added metacommands

codecov-commenter · 2021-08-29T14:37:00Z

Codecov Report

Merging #231 (a73eb8c) into main (5d8bb74) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   95.74%   95.81%   +0.07%     
==========================================
  Files          64       64              
  Lines        2750     2799      +49     
  Branches      412      424      +12     
==========================================
+ Hits         2633     2682      +49     
  Misses         75       75              
  Partials       42       42

Impacted Files	Coverage Δ
dask_sql/cmd.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d8bb74...a73eb8c. Read the comment docs.

nils-braun

Yeah! That is very nice, I like it. That will make the CLI very useful.
As we (or: you) are adding more and more functionality, I think we should also add some tests now. Do you have experience with testing CLI applications? I would probably use something like this. If you have never done this before: I am happy to add the first few tests to this PR (and you can take it from there if you want)

nils-braun · 2021-08-30T12:36:23Z

dask_sql/cmd.py

 from dask_sql.context import Context

+meta_command_completer = WordCompleter(
+    ["\l", "\d?", "\dt", "\df", "\de", "\dm", "\conninfo", "quit"]


You have the \d? command, but how about a \help¸ or ?` command also (with the same semantics)?

Later (not now), we can also add comments for connecting to another dask cluster or changing the schema.

I have added two metacommands

\dss Switch schema

\dsc Switch Dask cluster

let me know if this metacommands- name, and function make sense!

nils-braun · 2021-08-30T12:37:41Z

dask_sql/cmd.py

+            self.prompt = partial(prompt, **kwargs)
+
+
+def display_markdown(content, **kwargs):


Suggested change

def display_markdown(content, **kwargs):

def _display_markdown(content, **kwargs):

Let's keep it "private", users should not use this function. Can you also add a very small docstring?

Same also applies to parse_meta_command and meta_commands`.

nils-braun · 2021-08-30T12:39:13Z

dask_sql/cmd.py

+
+def display_markdown(content, **kwargs):
+    df = pd.DataFrame(content, **kwargs)
+    print(df.to_markdown(tablefmt="fancy_grid"))


Great, I love this - looks really good.
Unfortunately, it needs an additional package (tabulate). I see three possibilities:

try it, and on failure do a normal print

try it, and on failure do a normal print and print a warning

add the package to setup.py and make it a dependency.

I would favor 2 or 3 (with a small tendency towards 3), but its your choice!

Added tabulate in setup.py basically picked third option

nils-braun · 2021-08-30T12:40:30Z

dask_sql/cmd.py

+def display_markdown(content, **kwargs):
+    df = pd.DataFrame(content, **kwargs)
+    print(df.to_markdown(tablefmt="fancy_grid"))
+    return True


I see why you remove True (because you need it later as the result of meta_commands), but in principle this has nothing to do with display_markdown. So I would separate the concerns, do not have any return value here and return true in meta_commands.

makes perfect sense, !!!

nils-braun · 2021-08-30T12:41:06Z

dask_sql/cmd.py

+
+def parse_meta_command(sql):
+    command, _, arg = sql.partition(" ")
+    command = command.strip().replace("+", "")


Why replacing "+"? Did I miss something?

I am using pgcli to make the meta-commands, pgcli has support for specifying +, (if specified it will show some extra information like Description and size, etc ) so thought some people use the same notion as \dt+ etc. I have removed that now !!!

nils-braun · 2021-08-30T12:42:46Z

dask_sql/cmd.py

+    elif cmd == "\l":
+        return display_markdown(context.schema.keys())
+    elif cmd == "\dt":
+        return display_markdown(context.schema[schema_name].tables.keys())
+    elif cmd == "\df":
+        return display_markdown(context.schema[schema_name].functions.keys())
+    elif cmd == "\de":
+        return display_markdown(context.schema[schema_name].experiments.keys())
+    elif cmd == "\dm":
+        return display_markdown(context.schema[schema_name].models.keys())


Woa! That is really nice. I like this very clean code. Well done

nils-braun · 2021-08-30T12:43:18Z

dask_sql/cmd.py

+    elif cmd == "\dm":
+        return display_markdown(context.schema[schema_name].models.keys())
+    elif cmd == "\conninfo":
+        print("Dask cluster info")


That is the only command with a "title". Do we need it?

yeah removed it !!!

nils-braun · 2021-08-30T12:44:16Z

dask_sql/cmd.py

            text = session.prompt("(dask-sql) > ")
        except KeyboardInterrupt:
-            continue
+            sys.exit()


I actually like that Ctrl+C does not kill the program, but just gives me a new line. Is there a specific reason to change this? If people want to quit, they can use your new quit or just Ctrl+D.

Before I don't know the command ctrl+D closes the dask-sql-cli, so I thought of adding sys.exit at KeyboardInterrupt but now I removed that !!

nils-braun · 2021-08-30T12:48:02Z

dask_sql/cmd.py

-            print(df)
-        except Exception:
-            traceback.print_exc()
+        meta_command_detected = meta_commands(text, context=context, client=client)


Great!

One thing that I tried, was to enter an invalid meta-command (like \x). The tools will try to understand it as SQL command (which will give a not-so-useful error message). I think it would be nice to classify all lines starting with "" as meta-commands - and if they are now found show a useful error message and the help again. What do you think?

Good catch, I haven't thought about those inputs, now improved the error message!

nils-braun · 2021-08-30T13:14:37Z

dask_sql/cmd.py

+        if not meta_command_detected:
+            try:
+                with ProgressBar() as pb:
+                    for df in pb(sqlCaller(text), total=1, label="Executing"):


Really good idea, to add a progress bar. But I think we can do even better here - having a progress bar that actually depends on the tasks on the cluster, similar to Dask's progress bar.
Two ideas here. In both cases you need to call context.sql with return_futures=True, as we need to interact with the Dask tasks. To turn the dataframe into wait-able tasks, call .persist() (because a dataframe is a lazy action - nothing to wait on).
Either we then actually use dask's progress bar.
Or you use a combination of dask.distributed.as_competed and client.futures_of, something like (untested):

df = context.sql(text, return_futures=True) # (include None handling here) # Turn df into something awaitable: df = df.persist() # Now turn it into a list of futures futures = client.futures_of(df) # and finally show a progress bar: for df in pb(as_completed(futures), total=len(futures), label="Executing"): continue # In the end, show the actual result (which is now calculated, so that will be fast): df = df.compute() print(df.to_markdown(tablefmt="fancy_grid"))

Thanks for the Suggestion 👍 really great idea, copy-pasted the above solution and it worked as expected. !!

quasiben · 2021-08-30T19:24:13Z

I wanted to mention that @jacobtomlinson recently started using rich for the cli of https://github.com/dask-contrib/dask-ctl . Rich has fancy progressbars and colors among other attributes.

rajagurunath · 2021-09-01T10:15:03Z

I wanted to mention that @jacobtomlinson recently started using rich for the cli of https://github.com/dask-contrib/dask-ctl . Rich has fancy progressbars and colors among other attributes.

Thanks for the great suggestion @quasiben, will look into it and try to add those changes in upcoming PRs!

1.test cases for dask-sql-cli 2. Switch schema 3. Switch Dask cluster

rajagurunath · 2021-09-01T10:26:22Z

As we (or: you) are adding more and more functionality, I think we should also add some tests now. Do you have experience with testing CLI applications? I would probably use something like this. If you have never done this before: I am happy to add the first few tests to this PR (and you can take it from there if you want)

Thanks, @nils-braun for the quick review and suggestions.

I have added test cases but I think I kind of cheated 😅 , please review the test cases and let me know if something needs to be added or modified. (And I haven't tested the dask-sql-prompt Session as a whole but only tested _meta_commands function as of now, may need your help to test the prompt_toolkit session )

rajagurunath · 2021-10-12T11:25:06Z

rerun tests

charlesbluca

This looks really cool @rajagurunath 🙂 just some minor suggestions for typos/casing stuff, and a request to add the new tabulate dependency to another file:

setup.py

dask_sql/cmd.py

rajagurunath

Hi, Thanks a lot, @charlesbluca for reviewing this PR, Added a fix for the suggested changes, Please let me know if this change looks good! 🙂

charlesbluca

Thanks @rajagurunath! This looks good to me 😄

charlesbluca · 2021-10-20T20:13:03Z

continuous_integration/recipe/meta.yaml

    - prompt_toolkit >=3.0.8
    - pygments >=2.7.3
    - nest-asyncio >=1.0.0
+    - tabulate >=0.8.9


Just noting that we might change this pinning in a follow up once we do our next release - I want to match the recipe on conda-forge, so once this is added to the auto-generated recipe there I'll grab whatever pinning it has there (which may be the same). This should be good for now though 🙂

rajagurunath and others added 30 commits September 22, 2020 22:43

Merge pull request #1 from nils-braun/main

381939c

Increase Speed (dask-contrib#29)

Merge branch 'nils-braun:main' into main

3e106e3

ML model improvement : Adding "SHOW MODELS and DESCRIBE MODEL"

4a9ee68

Author: rajagurunath <[email protected]> Date: Mon May 24 02:37:40 2021 +0530

fix typo

f640ccc

ML model improvement : added EXPORT MODEL

ae76edd

ML model improvement : refactoring for PR

a484136

ML model improvement : Adding stmts in notebook

6e1709e

ML model improvement : Adding stmts in notebook

d15657d

ML model improvement : also test the non-happy path

4a3a9ee

ML model improvement : Added mlflow and <With> in sql for extra params

c722865

ML model improvement : Added mlflow and <With> in sql for extra params

d0feef3

Merge branch 'feature/export-ml-models' into feature/export-model

3a972da

Added Test cases for Export MODEL

e31956e

Added ML documentation about the following:

17a1514

1. SHOW MODELS 2. DESCRIBE MODEL 3. EXPORT MODEL

Merge pull request #3 from rajagurunath/feature/export-model

fbf93cb

Feature/export model

FETCH upstream changes

eb40f04

Merge branch 'nils-braun-main' into main

de7c993

Fetch upstream changes to resolve conflicts

FETCH upstream changes for export model

8886d10

refactored based on PR

e6f1e01

Merge pull request #6 from nils-braun/main

75888ce

Fix a failing build, as ciso8601 is currently not pip-installable (dask-contrib#192)

Added support only for sklearn compatible models

2ef98ea

Merge branch 'main' of https://github.com/rajagurunath/dask-sql into …

e173e24

…main

excluded mlflow part from code coverage

d3f03e1

install mlflow in test cluster

6b7fa8e

Added test for non sklearn compatible model

fff8dc4

get export model from main repo

ddaa0c0

Merge branch 'main' of https://github.com/nils-braun/dask-sql into ni…

e2bac80

…ls-braun-main

upstream merge

a6512c3

Merge branch 'nils-braun:main' into main

9028003

Merge branch 'nils-braun:main' into main

aadf500

rajagurunath added 5 commits August 14, 2021 20:12

Merge branch 'dask-contrib:main' into main

cd5f746

Merge branch 'dask-contrib:main' into main

2553002

Merge branch 'dask-contrib:main' into main

faef6da

Merge branch 'dask-contrib:main' into main

d917d35

improve dask-sql-cli:

e94fe7c

1. Autocompletion 2. AutoSuggestion 3. Added progressBar 4. Added metacommands

rajagurunath requested a review from nils-braun August 29, 2021 14:28

nils-braun reviewed Aug 30, 2021

View reviewed changes

Added

3b8d662

1.test cases for dask-sql-cli 2. Switch schema 3. Switch Dask cluster

Merge branch 'dask-contrib:main' into feature/improve_cli

46f48ec

Merge branch 'dask-contrib:main' into feature/improve_cli

fceacbb

quasiben mentioned this pull request Oct 13, 2021

[FEA] 0.4 Release ? #257

Closed

charlesbluca requested changes Oct 20, 2021

View reviewed changes

Merge branch 'dask-contrib:main' into feature/improve_cli

a73eb8c

rajagurunath commented Oct 20, 2021

View reviewed changes

Added PR suggestions and lint fix

87b306c

charlesbluca approved these changes Oct 20, 2021

View reviewed changes

charlesbluca reviewed Oct 20, 2021

View reviewed changes

charlesbluca merged commit b42db23 into dask-contrib:main Oct 20, 2021

quasiben mentioned this pull request Nov 3, 2021

[bot-automerge] dask-sql v0.4.0 conda-forge/dask-sql-feedstock#27

Merged

3 tasks

		self.prompt = partial(prompt, **kwargs)


		def display_markdown(content, **kwargs):

	def display_markdown(content, **kwargs):
	def _display_markdown(content, **kwargs):

Feature/improve cli #231

Feature/improve cli #231

Uh oh!

Conversation

rajagurunath commented Aug 29, 2021

Uh oh!

codecov-commenter commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nils-braun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quasiben commented Aug 30, 2021

Uh oh!

rajagurunath commented Sep 1, 2021

Uh oh!

rajagurunath commented Sep 1, 2021

Uh oh!

rajagurunath commented Oct 12, 2021

Uh oh!

charlesbluca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajagurunath left a comment

Choose a reason for hiding this comment

Uh oh!

charlesbluca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

codecov-commenter commented Aug 29, 2021 •

edited

Loading