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

refactor: use nix to manage development dependencies #2924

Merged
merged 2 commits into from
Dec 22, 2021

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Sep 1, 2021

This is a PR to change the way local development is done in Ibis (while preserving as much backwards compatibility with conda and non-conda/non-nix workflows as possible!), accomplishing the following things:

The main change here is the addition of a development environment that is self-contained and derived from a single source of truth (pyproject.toml), using nix.

See docs/web/contribute.md for more details.

Note that both conda and setuptools-based development workflows are still supported as first class citizens.

DONE:

  1. A move to poetry for dependency management. done in refactor(ci): move to poetry #2937
    It is still possible and supported to use setup.py and setuptools if you like, this functionality is checked in CI.
  2. More extensive CI testing, with minimal additional CI time used. It looks
    like roughly on average 3 minutes of additional CI time occur, with more of
    the library being tested on Windows in particular
    done in refactor(ci): move to poetry #2937
  3. Automatic generation of a conda recipe for every release, uploaded to GitHub
    releases (this is the remaining 1% that isn't automated but this can
    be automated by pushing a feedstock pull request directly from CI)
    I plan to do this in conda-forge directly, using their integrated bot automation
  4. Automated Dependabot updates for github-actions and poetry dependencies this is done using Renovate in Configure Renovate #3053
  5. Generating a setup.py file from the project's pyproject.toml file This is now just checked that it doesn't need to be regenerated, since we want to ensure this is tested in CI where relevant. done in ci: add workflow to automatically update setup.py #3054, necessary for Renovate dependency update PRs
  6. Automatic generation of a conda environment file, uploaded to GitHub releases, useful for developers using conda We have automatically generated lockfiles for linux, macos, and windows, for all three versions of Python currently supported (3.7, 3.8, 3.9). These live under conda-lock and can be used to set up a development environment very quickly since there's no solve step required. Done ci: generate conda lock files in CI #3077, ci: use git push instead of github action for lock file PR update commits #3080, ci: pull first when updating conda lock files #3083, ci: fix committing conda lock files from CI #3087

Follow ups:

  1. workflow_dispatch (click-to-run) GitHub Actions workflow that cuts a release to PyPI and GitHub Releases.
  2. Automatic license updates once a year or on-demand. This is optional, but it's another thing we can automate at some point.
  3. Automatic nix dependency updates via PR submission every six hours or on-demand. This has to be done in a follow up. Done in ci: add automatic dependency update PRs #3182

Happy to address any concerns here, let's automate everything!

@pep8speaks
Copy link

pep8speaks commented Sep 1, 2021

Hello @cpcloud! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-12-22 11:33:58 UTC

@cpcloud cpcloud force-pushed the nix-env branch 5 times, most recently from 575dd6f to bf9d3b1 Compare September 1, 2021 17:55
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you add a README.md explaining how the poetry2conda is used?

how are the min version set now for packages (e.g. where).

also is the release process documented somewhere? (even if fully automated)

.pre-commit-config.yaml Outdated Show resolved Hide resolved
docs/source/conf.py Outdated Show resolved Hide resolved

This may seem overly strict, but it takes a large amount of the release burden off
Copy link
Contributor

Choose a reason for hiding this comment

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

can you show more what is needed on commit messages?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, here's a screenshot of running cz (which replaces git commit) that shows a subset of what's possible:

image

Let's say I'm committing a feature, so I select feat. I'd get this prompt now:

image

Here you can put whatever component or filename you want, let's say I'm editing sql generation, I'd then get prompt to add a commit title:

image

You then get asked for a longer description which is optional unless there's a breaking change:
image

... breaking changes are the next question:
image

Finally you're asked about open issues the commit affects:
image

The commit conventions are taken from the Angular community: https://github.com/angular/angular.js/blob/master/DEVELOPERS.md#-git-commit-guidelines

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add this to the docs :-> (screen shots and everything). as this is a new workflow

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear, it isn't necessary to use cz, but it automates making the commit messages conform (which is now verified in CI).

Copy link
Contributor

Choose a reason for hiding this comment

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

i c.

ok then prob a good idea to show this workflow AND show that link above for the allowed messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, will add to the docs for sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is verified in CI then I would say it's necessary to use cz (which is fine), otherwise I can see the commit message doesn't pass CI easily.

ibis/__init__.py Outdated Show resolved Hide resolved
@jreback jreback added the ci Continuous Integration issues or PRs label Sep 1, 2021
@jreback
Copy link
Contributor

jreback commented Sep 1, 2021

cc @datapythonista

@cpcloud
Copy link
Member Author

cpcloud commented Sep 1, 2021

@jreback

can you add a README.md explaining how the poetry2conda is used?

Yup. In short it's used to generate an environment file in CI.

how are the min version set now for packages (e.g. where).

The minimum versions for dependencies are set in pyproject.toml

also is the release process documented somewhere? (even if fully automated)

Light documentation of it exists in docs/web/contributing..md, but I'll spell it out in more detail there.

ci/deps/pyspark-min.yml Outdated Show resolved Hide resolved
.prettierrc.toml Outdated Show resolved Hide resolved
@icexelloss
Copy link
Contributor

@cpcloud left some comments, in general +1 to standardize/automatic things so thanks for this!

My main concern is that I haven't used many of the tools here (I think some other folks as well), so would be really helpful to write dev docs describing how the new CI/dev/release works.

@cpcloud
Copy link
Member Author

cpcloud commented Sep 1, 2021

@cpcloud left some comments, in general +1 to standardize/automatic things so thanks for this!

My main concern is that I haven't used many of the tools here (I think some other folks as well), so would be really helpful to write dev docs describing how the new CI/dev/release works.

Heck yeah! @jreback has requested more docs as well. I think I'll also add a screencast to really help clarify how these things are used.

@cpcloud cpcloud force-pushed the nix-env branch 5 times, most recently from 331383c to 49a9d55 Compare September 2, 2021 13:26
ibis/tests/test_version.py Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
# Release Notes

<!--next-version-placeholder-->
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI this is where new release notes based on commit logs will start to be automatically populated upon release.

@cpcloud
Copy link
Member Author

cpcloud commented Sep 2, 2021

@jreback @icexelloss I've added a bunch of docs on the dev and release process. Please review at your earliest convenience!

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

umm why all the references to 'breaking changes' here? why is this in any way controlled by the commits that someone pushes? how could they possibly know if something is a breaking change or not?

sure the maintainers can know / understand this, but these are general instructions.

.github/workflows/ibis-backends.yml Show resolved Hide resolved
If you don't have a database for the backend you want to work on, you can check the configuration of the
continuos integration, where docker images are used for different backend. This is defined in
`.github/workflows/main.yml`.
Once you're inside of a nix-shell, the first thing you want to do is make a branch. Let's call it `useful-bugfix`.
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is not specific to nix, can you be more generic here

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more descriptive in what you're asking for?


## Commit philosophy
**NOTE:** Ensure that you have run `pre-commit run` **before** running `cz`, otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

is the pre-commit still available? i think you removed the config?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I've already explained this elsewhere in the PR.

We use [numpydoc](https://numpydoc.readthedocs.io/en/latest/format.html) as
our standard format for docstrings.
- required: `git`
- required: `cz` (available in the `nix` environment)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a doc link on cz?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what you're asking. Do you want me to put a link to the cz repository/documentation?

docs/web/contribute.md Show resolved Hide resolved
@cpcloud
Copy link
Member Author

cpcloud commented Dec 20, 2021

@jreback

umm why all the references to 'breaking changes' here?

It seems like you might not have had a chance to take a look at semantic-release? In particular this section answers this question.

why is this in any way controlled by the commits that someone pushes?

The answer to this question is also in the above links, but I'll give a brief summary.

The point of semantic release is to entirely automate the drudgery of release. This is done by giving semantics to commit messages, e.g., feat is a feature (causing a minor version bump), bug is a bug causing a patch version bump. The structure is enforced by cz (optional to use) and enforced by the commitlint check in CI (not optional).

I think you may be missing the fact that these version determinations are batched at release time: there won't be a new release per commit, that would be excessive.

Instead, the commits between the last release and the current commit are examined for feat, bug etc. If feat is found, it's a minor bump (from say 2.0.0 to 2.1.0), if bug is found it's a patch bump (from say 2.0.0 to 2.0.1). There are a few other rules (notably any message with BREAKING CHANGE: blah blah blah triggering a major version bump) but what I just wrote captures most of the important information.

All of this is covered in the semantic-release repo, please take a look at that.

Also note that this PR doesn't implement the release automation. I explicitly state that it'll be done in a follow up in the PR description. Did you read that? Please do so if you haven't.

how could they possibly know if something is a breaking change or not?

Not sure who they is here, but if they means "a developer", then those are the only people who can know what breaks and what doesn't before a release.

If by they you mean "commit messages", then again, that's covered in the semantic-release repo link above.

@jreback
Copy link
Contributor

jreback commented Dec 20, 2021

by they i mean - a person pushing commits

they cannot possibly know what is. breaking change or not

the maintainers could know but o am not sure how this helps

@cpcloud
Copy link
Member Author

cpcloud commented Dec 20, 2021

by they i mean - a person pushing commits

they cannot possibly know what is. breaking change or not

the maintainers could know but o am not sure how this helps

Not sure what the objection is here. We don't have random people making commits. Every change goes through review. If a change needs a commit to indicate a breaking change, then it's up to maintainer to make sure that the commit has it. Typically this would be done by referring someone to the docs.

@jreback
Copy link
Contributor

jreback commented Dec 20, 2021

If a change needs a commit to indicate a breaking change, then it's up to maintainer to make sure that the commit has it.

this seems like quite a lot of work - meaning the commits need to be exactly right

having release notes as part of the change is way easier

@cpcloud
Copy link
Member Author

cpcloud commented Dec 20, 2021

If a change needs a commit to indicate a breaking change, then it's up to maintainer to make sure that the commit has it.

this seems like quite a lot of work - meaning the commits need to be exactly right

having release notes as part of the change is way easier

It's not a lot of work, and cz handles it. I've used this procedure at both past companies and in my own projects and whatever small overhead there is in making your commits more structured is easily made up by the ability to release with confidence, whenever we want. cz makes this process even easier because it'll prompt you, and construct the commit message correctly. Check out the prompts I show above, which you explicitly requested.

It might be useful to try it out, so that if you have specific objections I can address those.

We don't have that many breaking changes.

We do however need to release faster, and it needs to be a super boring and routine procedure that anyone can do with a single click.

I would like to move forward with this unless there specific actionable objections, in which case I will address them.

@jreback
Copy link
Contributor

jreback commented Dec 20, 2021

ok will look again tomorrow

in any event would like to make the docs super clear about this

@jreback
Copy link
Contributor

jreback commented Dec 21, 2021

btw likely we are plaguaged by pypa/setuptools#2941 (to mitigate can pin < 60)

@cpcloud cpcloud force-pushed the nix-env branch 2 times, most recently from a56923b to 78d47e9 Compare December 21, 2021 17:06
@cpcloud
Copy link
Member Author

cpcloud commented Dec 21, 2021

btw likely we are plaguaged by pypa/setuptools#2941 (to mitigate can pin < 60)

Thanks. For now it looks like this PR isn't affected.

@jreback On a different note, I've reworked the docs and made it even easier to use cz (it's now a dev-dependency). Let me know if there's anything else you'd like to me to add to the contributing.md docs.

@jreback
Copy link
Contributor

jreback commented Dec 21, 2021

kk will look shortly

@cpcloud
Copy link
Member Author

cpcloud commented Dec 21, 2021

/condalock

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

seems fine to merge on green.

skeptical about the auto release notes but that's in the future anyhow.

docs/web/contribute.md Outdated Show resolved Hide resolved
# if using nix
$ ./dev/poetry2setup -o setup.py

# it not using nix, requires installation of tomli and poetry-core
Copy link
Contributor

Choose a reason for hiding this comment

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

these are in the conda files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not explicitly, but they are both a pip install/conda install away.

@cpcloud
Copy link
Member Author

cpcloud commented Dec 22, 2021

/condalock

@cpcloud cpcloud modified the milestones: 2.x, 3.x Dec 22, 2021
@cpcloud
Copy link
Member Author

cpcloud commented Dec 22, 2021

Thanks.

I'm going to turn off the pre-commit.ci integration and move us to rebase and merge for merging. commitlint will take care of notification of non-conforming commit messages

@cpcloud cpcloud merged commit aa31138 into ibis-project:master Dec 22, 2021
@cpcloud cpcloud deleted the nix-env branch December 22, 2021 13:48
@jreback
Copy link
Contributor

jreback commented Dec 22, 2021

thanks @cpcloud great

@ibis-project-bot
Copy link
Contributor

🎉 This PR is included in version 2.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration issues or PRs dependencies Issues or PRs related to dependencies developer-tools Tools related to ibis development
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants