Generate notebook sources #301
Conversation
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 95 95
Lines 4780 4780
Branches 456 456
=======================================
Hits 4471 4471
Misses 309 309 Continue to review full report at Codecov.
|
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
docs/dev/gen-nb-src.py
Outdated
|
|
||
| print("Generating source file for {} under {} ...".format(html, SOURCE_DIR)) | ||
|
|
||
| def run_cli_workspace_cmd(args): |
There was a problem hiding this comment.
Why is this a nested definition?
There was a problem hiding this comment.
This is just so we can leverage cli_profile from the main args, but I can split this out if it's clearer that way.
There was a problem hiding this comment.
I think that would be a bit cleaner.
docs/dev/gen-nb-src.py
Outdated
| source_path = os.path.join(SOURCE_DIR, rel_path + "." + ext) | ||
| if os.path.exists(source_path): | ||
| os.remove(source_path) | ||
| print("\tDeleted {}.".format(source_path)) |
There was a problem hiding this comment.
We printed a line about the HTML file before this; here, we add a line for all associated generated source files (although reasonably, there should only be 1).
docs/dev/gen-nb-src.py
Outdated
|
|
||
| @click.command() | ||
| @click.option('--html', required=True, help='Path of the HTML notebook.') | ||
| @click.option('--cli-profile', default='docs-ci', help='Databricks CLI profile name.') |
There was a problem hiding this comment.
Might it make more sense to rely on the databricks CLI's default profile?
There was a problem hiding this comment.
I found this useful during manual testing, as I already have a default Databricks CLI profile and wanted to test this on the same shard as the CI.
There was a problem hiding this comment.
I mean that it makes sense to have this argument, but the default value should be the default profile. If you're running locally, I don't think there's any reason to expect a profile called docs-ci to exist. We can pass the docs-ci argument from our CI script.
docs/dev/gen-nb-src.py
Outdated
| @click.command() | ||
| @click.option('--html', required=True, help='Path of the HTML notebook.') | ||
| @click.option('--cli-profile', default='docs-ci', help='Databricks CLI profile name.') | ||
| @click.option('--workspace-tmp-dir', default='/tmp/glow-docs-ci', help='Temp workspace dir for import/export.') |
There was a problem hiding this comment.
We should probably generate a temporary directory if one is not provided. In fact, maybe we should always just make a new one?
There was a problem hiding this comment.
Actually, we do generate a temporary work directory under this; this serves as the root directory. I'll clarify in the description.
Signed-off-by: Karen Feng <[email protected]>
docs/dev/gen-nb-src.py
Outdated
|
|
||
| @click.command() | ||
| @click.option('--html', required=True, help='Path of the HTML notebook.') | ||
| @click.option('--cli-profile', default='docs-ci', help='Databricks CLI profile name.') |
There was a problem hiding this comment.
I mean that it makes sense to have this argument, but the default value should be the default profile. If you're running locally, I don't think there's any reason to expect a profile called docs-ci to exist. We can pass the docs-ci argument from our CI script.
docs/dev/gen-nb-src.py
Outdated
|
|
||
| print("Generating source file for {} under {} ...".format(html, SOURCE_DIR)) | ||
|
|
||
| def run_cli_workspace_cmd(args): |
There was a problem hiding this comment.
I think that would be a bit cleaner.
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
Signed-off-by: Karen Feng <[email protected]>
python/environment.yml
Outdated
| - pandas=0.25.3 | ||
| - pip | ||
| - pyarrow=0.15.1 | ||
| - pyspark=3.0.0 | ||
| - python=3.7 |
There was a problem hiding this comment.
I think it's actually standard to list python first since it is pretty important :).
Signed-off-by: Karen Feng <[email protected]>
What changes are proposed in this pull request?
Adds generated notebook sources for our documentation for easy diff-ability by converting from HTML to source code. Checks via CI that the generated sources are not out-of-date.
How is this patch tested?
To generate a notebook source yourself:
gen-nb-src.pywith the input HTMLExample of CI catching failures: https://app.circleci.com/pipelines/github/projectglow/glow/1646/workflows/58e2deeb-7036-468e-8975-bcc3115a04a0/jobs/3878