-
Notifications
You must be signed in to change notification settings - Fork 15
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
Training upgrade #139
Training upgrade #139
Conversation
4707858
to
08141e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising! 👏
Note: after merging, we'll need to switch the main branch from |
Did you see #109? Could it be (have been) helpful? |
Co-authored-by: Matti Schneider <matti.schneider@beta.gouv.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The version number is not reinitialised after the bootstrap script is executed, meaning the first version number is 7.0.0.
I tried executing, and got the following:
test/country-template-training-upgrade [main]
› python --version
Python 3.11.7
test/country-template-training-upgrade [main]
› pip install openfisca-senegal
Collecting openfisca-senegal
Downloading OpenFisca_Senegal-0.10.3-py3-none-any.whl.metadata (1.5 kB)
Collecting OpenFisca-Core<36.0,>=35.0.1 (from openfisca-senegal)
…
Downloading wcwidth-0.2.13-py2.py3-none-any.whl (34 kB)
Building wheels for collected packages: numpy
Building wheel for numpy (pyproject.toml) ... error
error: subprocess-exited-with-error
× Building wheel for numpy (pyproject.toml) did not run successfully.
│ exit code: 1
╰─> [1264 lines of output]
setup.py:66: RuntimeWarning: NumPy 1.20.3 may not yet support Python 3.11.
We insist that Python version should be 3.9, but give no guidance on how to install that specific version. We can consider that this is out of scope, but “should print "Python 3.9.xx"” is a bit light compared to “Install Python 3.9” with a link to how to install that version.
In my case (macOS), I could make it work by using brew install python@3.9
and then using pip3.9
instead of pip
.
As written in the README, I tried to run openfisca serve --port 5000
after installing, and that failed with openfisca: command not found
. I used instead make serve-local
which is suggested as an alternative, and it worked. If we cannot make openfisca serve
work, let's remove that instruction.
## 7.0.0 [#139](https://github.com/openfisca/country-template/pull/139) | ||
|
||
* Technical improvement. | ||
* Major change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any breaking change, so according to semver this could just be a minor 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is the breaking change discussed between this and OpenFisca-Core tests...
bootstrap.sh
Outdated
read JURISDICTION_NAME | ||
if [[ "$JURISDICTION_NAME" != "" ]] | ||
then | ||
lowercase_jurisdiction_name=$(echo $JURISDICTION_NAME | tr '[:upper:]' '[:lower:]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to make this even more forgiving, we should remove diacritics. For example, if I type Sénégal
as a jurisdiction, this should convert to senegal
, as otherwise I get:
› pip install openfisca-sénégal
ERROR: Invalid requirement: 'openfisca-sénégal'
If this is too costly to implement, we probably should clarify “without accents” in the read
prompt 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented and tested possible solution, please see commit #f38f8f9
bootstrap.sh
Outdated
echo '************' | ||
echo '* All set! *' | ||
echo '************' | ||
exec bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This starts a bash
session without loading .profile
and other session-specific setup, which is very confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have not found another solution automatically orientating the user to the newly created directory. I'm open to suggestions on this. We can revert which leaves them at a bash prompt located in a non existent folder which might also be confusing.
Co-authored-by: Matti Schneider <matti.schneider@beta.gouv.fr>
…ME.md improvements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these changes!
I realised the current implementation removes the ability to use env variables, which prevents automated execution. We should instead ask interactively only if it is not readily available.
I'll take over the last changes 🙂
Exit with code 3 in case of user interruption
Example: Virgin Islands
The profile files are not loaded in this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made several changes, please double-check them before merging 🙂
The remote over HTTPS needs constant authentication and user would need to remove and then re-add the remote
Was silent in https://github.com/openfisca/country-template/actions/runs/8810580013, which led to failed merge in #139
bootstrap.sh
to improve experience for first time users:COUNTRY_NAME
andREPOSITORY_URL
before running.master
branch tomain
by defaultCONTRIBUTING.md
to allow for improved text replacementsmaster
withmain
Makefile
OpenFisca France
replace with new repository name-e
with longer form--editable
to aid newcomersisort
,pyupgrade
andyamllint
pyproject.toml
based on the oldersetup.py
andsetup.cfg
[project.urls]
:Homepage
,Repository
,Documentation
,Issues
andChangelog
[project.optional-dependencies]
: new dependanciesisort
,pyupgrade
andyamllint
[tool.pytest.ini_options]
with filterwarnings set to"error"
[tool.pylint.messages_control]
fromsetup.cfg
but in long form for readability[tool.isort]
section.flake8
and.yamllint
to project root as these projects do not supportpyproject.toml
formatREADME.md
to allow for improved text replacementsexport COUNTRY_NAME=France
andexport REPOSITORY_URL=...
due to bootstrap.sh improvementsgit push origin master
togit push origin main
setup.py
andsetup.cfg
.github/*.sh
and.github/workflows/workflow.yaml
files to referencemain
instead ofmaster
openfisca_country_template/texts/social_security_contribution.yaml
openfisca_country_template/texts/situations/income_tax.yaml
openfisca_country_template/variables/demographics.py
openfisca_country_template/variables/taxes.py
CHANGELOG.md
, add example entry and alterbootstrap.sh
to strip out country-template entries. This resolves issue 116.Note that this has been tested against OpenFisca-core and requires corresponding pull request to pass the OpenFisca-core test suite.
If this is approved, it is the intention to upgrade the documentation surrounding OpenFisca which will likely include the *.md files in this repository and result in a subsequent PR.