Skip to content

Fresh setup #198

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

Merged
merged 7 commits into from
Sep 26, 2023
Merged

Conversation

antoinejeannot
Copy link
Contributor

Hi 👋
This PR attempts to clean things a bit concerning the dev, CI and releases workflows.

It tries to stay ISO-functional while adding some niceties:

While working on this PR, several issues have been encountered but not yet solved:

  • some FIXME & TODO have been added at appropriate places (mostly for cool pre-commit hooks that fix too many things for one PR)
  • tensorflow's dependencies policy (with strict higher-bounds) prevents us from having one [dev] optional (hence the need to properly install tensorflow via a side-step in the nox session)
  • the docs issue has not been handled yet, fortunately a next PR will
  • tests depending on optionals (assets-X, tensorflow etc) are only carried out on Ubuntu. While it does not seem right at first glance (except to gain some CI time ofc), it has already led to some issues such as the just merged Add get_keras_model to TensorflowModel #187 which did not build on ubuntu but passed everywhere else, which left a peer in disarray.

Looking forward to your feedback on this one,
Feel free to review it commit by commit, as I tried to split the work as much as possible 🙏

- drop setup.py & setup.cfg
- use pyproject.toml
- remove deprecated config code
bump2version being deprecated, bump-my-version
is a maintained fork.
- add pre-commit
- use black, ruff & mypy via pre-commit
- fix some lint issues catched by ruff
- drop scripts folder
- add Makefile
- add dev optional dependency
- remove all requirements files
- generate new requirements-dev.txt
- debug-statements
- vulture (and drop RUF100
@tbascoul
Copy link
Collaborator

tbascoul commented Sep 21, 2023

I would argue against keeping the requirements-dev.txt
I understand the reproducibility aspect of it, but what is most likely to happen is that we will forget about it and end up running the CI on deprecated packages.
As you are already familiar with filelock project 😄 , here is an example of a setup with only a pyproject file: https://github.com/tox-dev/filelock/blob/c1163ae57128cb398a70c3dce3bfd816fc3599f0/pyproject.toml#L46
I find it better to have a minimum version requirement in pyproject optional dependencies.
It might break sometimes but at least you are keeping up to date with other packages.

Edit: after digging a bit more, the method used in httpx project might be the one I prefer: https://github.com/encode/httpx/blob/master/requirements.txt#L1-L5
Pinned requirements for the testing/doc/packaging part, non-pinned for all other packages.

@antoinejeannot
Copy link
Contributor Author

I would argue against keeping the requirements-dev.txt I understand the reproducibility aspect of it, but what is most likely to happen is that we will forget about it and end up running the CI on deprecated packages. As you are already familiar with filelock project 😄 , here is an example of a setup with only a pyproject file: https://github.com/tox-dev/filelock/blob/c1163ae57128cb398a70c3dce3bfd816fc3599f0/pyproject.toml#L46 I find it better to have a minimum version requirement in pyproject optional dependencies. It might break sometimes but at least you are keeping up to date with other packages.

Edit: after digging a bit more, the method used in httpx project might be the one I prefer: https://github.com/encode/httpx/blob/master/requirements.txt#L1-L5 Pinned requirements for the testing/doc/packaging part, non-pinned for all other packages.

Totally fair point. As discussed on the side, I was tempted to drop the requirements-dev.txt file as well before reconsidering it to avoid too much change in the CI / dev workflow.

That said, I 💯 agree with the deprecation of the requirements-dev.txt, in favor of a test-requirements.txt with tests,docs,packaging dependencies hard-pinned.

@antoinejeannot antoinejeannot force-pushed the fresh-setup branch 2 times, most recently from 40aa993 to dbb84ed Compare September 21, 2023 12:59
- only hard-pin test dependencies
- add/update/drop Makefile recipes
- adapt github workflows
@antoinejeannot
Copy link
Contributor Author

PTAL @tbascoul

@antoinejeannot antoinejeannot merged commit 1db6781 into Cornerstone-OnDemand:main Sep 26, 2023
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.

2 participants