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

Pre-commit hooks for notebook formatting & reproducibility #22

Merged
merged 10 commits into from
Feb 26, 2021

Conversation

bdice
Copy link
Member

@bdice bdice commented Feb 18, 2021

Description

This PR adds pre-commit checks that should make notebooks a bit more reproducible and improves & automates notebook cell formatting. Metadata is no longer committed, which is a good thing. I verified that the tags discussed in #21 related to nbsphinx hiding certain cells and continuing execution after a cell failure are preserved.

resources:
  repositories:
    - repository: asottile
      type: github
      endpoint: github
      name: asottile/azure-pipeline-templates
      ref: refs/tags/v2.1.0

jobs:
- template: job--pre-commit.yml@asottile
  parameters:
  - python: "3.8"

Checklist:

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bdice bdice changed the title Pre commit Pre-commit hooks for notebook formatting & reproducibility Feb 18, 2021
@bdice bdice requested a review from joaander February 18, 2021 22:27
@bdice bdice self-assigned this Feb 18, 2021
@bdice bdice added the enhancement New feature or request label Feb 18, 2021
@bdice bdice changed the base branch from master to v3-api February 18, 2021 22:27
@joaander
Copy link
Member

Yes, I've configured an endpoint like that for the hoomd azp config that posts releases. You may be able to reuse the existing endpoint.

@joaander
Copy link
Member

Yes, I've configured an endpoint like that for the hoomd azp config that posts releases. You may be able to reuse the existing endpoint.

Or maybe not, they appear to be per project. The ones I've configured are "release" in gsd, hoomd, and fresnel.

@bdice
Copy link
Member Author

bdice commented Feb 19, 2021

@joaander The CI check seems to work! It's fine with me if you want to merge this without waiting for the "execute notebooks" step, or combine this into #21.

@joaander joaander requested a review from b-butler February 19, 2021 17:22
Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks!

Does nbqa work with any formatters other than black?

jobs:
- template: job--pre-commit.yml@asottile
Copy link
Member

Choose a reason for hiding this comment

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

So we are running a job that is define by a .yml file in asottile/azure-pipeline-templates. What if this repo changes the file, or goes stale? We just copy the file in to our templates directory and remove the resources block to keep control.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's tagged to a release so this file should not be expected to change. I would prefer to let it use the external template for simplicity. ref: refs/tags/v2.1.0

" ]\n",
" scene.camera = fresnel.camera.orthographic(\n",
" position=(0, 0, L + 1), look_at=(0, 0, 0), up=(0, 1, 0), height=L + 1\n",
" )\n",
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, black makes code blocks like this harder to read.

"outputs": [],
"source": [
"m = 5\n",
"N_particles = 2 * m**3"
"N_particles = 2 * m ** 3"
Copy link
Member

Choose a reason for hiding this comment

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

Spaces around the exponent operator make it hard to read.

@bdice
Copy link
Member Author

bdice commented Feb 22, 2021

Does nbqa work with any formatters other than black?

Yes, nbqa is flexible. I suspect yapf will work if that's your preference. If you have a yapf settings file or other configuration you want to use, let me know.

@joaander
Copy link
Member

Does nbqa work with any formatters other than black?

Yes, nbqa is flexible. I suspect yapf will work if that's your preference. If you have a yapf settings file or other configuration you want to use, let me know.

This is the yapf config I use with HOOMD:

[yapf]
based_on_style = google
align_closing_bracket_with_visual_indent = True
split_before_arithmetic_operator = True
split_before_bitwise_operator = True
split_before_logical_operator = True
blank_line_before_module_docstring = True
split_before_dot = True

You may want to wait until we merge #21 which rewrites many of the notebook files and accept merge those incoming changes before rerunning the formatting.

@bdice bdice mentioned this pull request Feb 25, 2021
3 tasks
@bdice
Copy link
Member Author

bdice commented Feb 25, 2021

@joaander I updated this PR and I think it's using yapf correctly. I copied the setup.cfg directly from HOOMD master and created a PR adding yapf to nbQA: nbQA-dev/nbQA#547

Copy link
Member

@joaander joaander left a comment

Choose a reason for hiding this comment

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

Thanks!

@joaander joaander merged commit 6d6cc95 into v3-api Feb 26, 2021
@joaander joaander deleted the pre-commit branch February 26, 2021 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants