Open MattiSG opened 3 years ago
The status quo is acceptable. It is not worth investing in changing something that works, the problems identified mostly concern maintainers, who are in a limited number and can learn to cope with that complexity and decrease it slowly when opportunities arise.
All workflow steps are declared in one place, and referenced elsewhere as needed. This can be achieved through make
or a more modern alternative such as invoke
or a combination of tox
/ nox
and poetry
(cf. https://github.com/openfisca/openfisca-core/pull/1036#issuecomment-910292534).
$tool check-version-number
and $tool has-functional-changes
, for example.$tool install
, $tool test
, $tool build
, $tool publish
.CI is the main way all tests are executed, and is the most precise way to orchestrate the entire workflow, relying on a well documented syntax. With the migration to GitHub Actions (#1030), we have the opportunity to use act
to execute all or part of the workflow locally.
pip install
) and the CLI (openfisca test
, openfisca serve
) for their usual needs.act
to execute CI targets for packaging, relying on Docker.The CLI is maintained and possibly improved for users who simply install OpenFisca as a dependency, through its Web or Python API. At the same time, contributors are directed to a task manager. CI config barely orchestrates the tasks declared in the task manager.
$tool test
would redirect to openfisca test
, passing all options).pip install
, openfisca test
, openfisca serve
.$tool install
, $tool test
, $tool build
.$tool check-version-number
and $tool has-functional-changes
, for example.All workflow steps are declared in one place, and referenced elsewhere as needed. This can be achieved through
make
or a more modern alternative such asinvoke
or a combination oftox
/nox
andpoetry
(cf. #1036 (comment)).
make
is not easily available on Windows.
tox
, nox
and poetry
are very nice tool but the learning curve for non expert (including myself ;-)) might be rough.
invoke
seems a good compromise since it is python based and may be used in interaction with the CLI.
I'm pretty much in favour of proposition 3 :)
@maukoquiroga thanks for your enthusiasm! :smiley: In order to minimise noise though, I encourage all participants to focus on using emoji reactions to express their preferences, and to use comments to add details and counter-propositions, as done above 🙂 Thank you, and sorry for being a killjoy 😉
Reminder: this RFC will close in 48 hours, unless the proposition set changes 🙂 Please cast your vote!
Just fyi I tested Act
on Openfisca France
and it did not bode well for me personally nor for @sandcha. Here is a summary.
Thanks @MattiSG! So to add details as what my perfect solution could look like (mostly proposition 3 but a bit of 1):
$ make install
[⚙] Install project dependencies...
Let's call it openfica
, example usages:
$ openfica --help
Usage: openfica <subcommand> [--subcommand-opts] ...
Subcommands:
serve Run the OpenFisca Web API.
test Run OpenFisca tests.
make.api Serve the openfisca Web API.
make.check-style Run linters to check for syntax and style errors.
make.check-syntax-errors Compile python files to check for syntax errors.
make.check-types Run static type checkers for type errors.
make.clean Delete builds and compiled python files.
make.format-style Run code formatters to correct style errors.
make.test Run openfisca-core tests.
$ openfica --help serve
Usage: openfica serve [--options]
Docstring:
Run the OpenFisca Web API.
Examples:
$ openfica serve --country-package openfisca_country_template
Options:
-c STRING, --country-package=STRING Country package to use.
-e [STRING], --extensions[=STRING] Extensions to load.
$ openfica serve --country-package openfisca_country_template
[2021-09-08 21:08:23 +0200] [56907] [INFO] Starting gunicorn 20.1.0
[2021-09-08 21:08:23 +0200] [56907] [INFO] Listening at: http://127.0.0.1:5000 (56907)
[2021-09-08 21:08:23 +0200] [56907] [INFO] Using worker: sync
[2021-09-08 21:08:23 +0200] [56939] [INFO] Booting worker with pid: 56939
[2021-09-08 21:08:23 +0200] [56940] [INFO] Booting worker with pid: 56940
[2021-09-08 21:08:23 +0200] [56941] [INFO] Booting worker with pid: 56941
$ openfica --help test
Usage: openfica test [--options]
Docstring:
Run OpenFisca tests.
Examples:
$ openfica test "$(git ls-files 'tests/**/*.py')"
Options:
-p STRING, --path=STRING Paths (files or directories) of tests to execute.
-w STRING, --workers=STRING
$ openfica test "$(git ls-files 'tests/**/*.py')"
.................................................................................................................................................................................................... [ 40%]
....................................................................................................................................................................x............................... [ 81%]
.......................................................................................... [100%]
481 passed, 1 xfailed, 151 warnings in 6.67s
openfica
also provides a make-like API:
$ openfica make.clean make.check-syntax-errors make.check-style make.check-types make.test
[⚙] Delete builds and compiled python files...
[⚙] Compile python files to check for syntax errors...
[⚙] Run linters to check for syntax and style errors...
[⚙] Run static type checkers for type errors...
Success: no issues found in 131 source files
[⚙] Run openfisca-core tests...
.................................................................................................................................................................................................... [ 40%]
....................................................................................................................................................................x............................... [ 81%]
.......................................................................................... [100%]
481 passed, 1 xfailed, 151 warnings in 6.84s
Good old make
to the rescue:
## Launch all `openfica` tasks.
all: \
openfica-clean \
openfica-check-syntax-errors \
openfica-check-style \
openfica-check-types \
openfica-test \
;
openfica-%:
@openfica make.$*
And then ✨ :
$ time make -j 1
[⚙] Delete builds and compiled python files...
[⚙] Compile python files to check for syntax errors...
[⚙] Run linters to check for syntax and style errors...
[⚙] Run static type checkers for type errors...
Success: no issues found in 131 source files
[⚙] Run openfisca-core tests...
.................................................................................................................................................................................................... [ 40%]
....................................................................................................................................................................x............................... [ 81%]
.......................................................................................... [100%]
481 passed, 1 xfailed, 151 warnings in 6.71s
real 0m13.114s
user 0m17.181s
sys 0m2.432s
$ time make -j `nproc`
[⚙] Compile python files to check for syntax errors...
[⚙] Delete builds and compiled python files...
[⚙] Run openfisca-core tests...
[⚙] Run linters to check for syntax and style errors...
[⚙] Run static type checkers for type errors...
Success: no issues found in 131 source files
.................................................................................................................................................................................................... [ 40%]
....................................................................................................................................................................x............................... [ 81%]
.......................................................................................... [100%]
481 passed, 1 xfailed, 151 warnings in 7.92s
real 0m9.444s
user 0m20.110s
sys 0m3.200s
You can try a working prototype here 🥳
Thanks everyone for your participation! ☺️
Proposition 3 is both the most consensual and the most supported, with 5 :+1: and 0 :-1: . On top, @HAEKADI's detailed exploration demonstrates that proposition 2 would be impossible to implement.
Thus, Proposition 3: “CLI for users, task manager for contributors” is adopted.
I'll lock this conversation in order to lock votes. Implementation choices will move to the open issues and PRs, and to new ones if some elements are not already covered 🙂
The data from the survey contains some interesting learnings, which I'll highlight below. Feel free to examine it and share your own learnings! I suggest that contribution number 6 should be excluded from analysis as it contains several answers that are impossible (using the OpenFisca CLI to deploy and uninstall the package).
Two things that I learned:
make
for one task, they are much more likely to use make
for other tasks.The distribution in usage shows that:
pip
.make
.make
task available, it is preferably applied in CI.I believe this should make us adjust slightly the proposal in that the CONTRIBUTING
file should only reference the task runner when there is no CLI command already available.
RFC: Consolidate build workflow
Context
There are currently four sources of truth for how to build, test and deploy an OpenFisca package:
.circleci/config.yml
, soon.github/workflows/config.yml
, cf. #1030)..circleci/*.sh
, soon.github/*.sh
).Makefile
.Depending on each case, these complement (installing locally, testing both locally and in CI, packaging in CI), reference (
publish-git-tag.sh
from CI config,openfisca test
intest-api.sh
) or overlap (make check-style
vs.circleci/lint-changed-python-tests.sh
) each other.Problem
This setup raises the following issues:
make install
differs from the install result in CI, which callsmake build
instead), leading to inconsistent test results (e.g. https://github.com/openfisca/openfisca-core/pull/1037).make test
and contributorsopenfisca test
), leading to blocked PRs (e.g. https://github.com/openfisca/openfisca-core/pull/1021#issuecomment-852404640).Discussion
A synchronous discussion with @benjello @benoit-cty @sandcha @maukoquiroga @HAEKADI @MattiSG took place on 02/09/2021 on this topic.
Conclusions on that topic were that:
make
overopenfisca
as it is shorter, especially when multiple country packages are installed in one virtualenv (make test
vsopenfisca test --country-package openfisca_france tests
).Annex
Known workflow steps
The following table aims at summarizing all known declared workflow steps. It was established by @MattiSG on 03/09/2021, based on reading through Core, France and Country Template
Makefile
,.circleci/config.yml
and.circleci/*.sh
.pip install --editable
(optional)make deps
,make install
pip install openfisca_country_package
lint-changed-python-files.sh
,lint-changed-yaml-tests.sh
make check-syntax-errors check-style
make format-style
pytest && openfisca test
make test
openfisca test
make api
(in Core)make serve-local
(in Country Template)openfisca serve
git fetch && check_version_and_changelog.sh
check_version_and_changelog.sh
make build
make uninstall
,make clean
test-api.sh
publish-python-package.sh && publish-git-tag.sh
publish-python-package.sh && publish-git-tag.sh
make check-types
Usage survey
In parallel to this RFC, in order to identify which tasks might be removed altogether from the list of existing ones, and which destination is least likely to disrupt existing uses, a survey has been created by @MattiSG to better understand the current usages. You are strongly encouraged to participate to it: share your current workflow.
The results of the survey are publicly available.
Process
This RFC aims at finding the best solution to the problems identified above. Contributors are invited to vote on the propositions below, which will be exposed each in their own post, through emoji reactions (:+1: to support an option, :-1: to reject it, :eyes: to indicate having read it and not supporting nor rejecting it). Contributors are also invited to ask for clarifications, suggest improvements to the propositions if that could change their stance on them, and contribute their own if they believe they can offer significant improvements.
In order to contribute a proposition, please make clear whether it is a new one, in which case you should give it a new number (e.g. “proposition 5”), or if it builds upon an existing one to add some detail, in which case you should suffix the original number with a letter (e.g. “proposition 3A”).
This RFC will close after one week (7 ⨉ 24 hours) without changes to the proposition set. At that time, votes will be counted and the most consensual proposition will be implemented.