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

Update dependencies #264

Merged
merged 10 commits into from
Jun 17, 2022
Merged

Update dependencies #264

merged 10 commits into from
Jun 17, 2022

Conversation

MattiSG
Copy link
Member

@MattiSG MattiSG commented May 10, 2022

Updates all dependencies to latest version. Supersedes #259. Fixes incompatible Jinja version issue encountered upon installing.

In particular, recommonmark is obsolete and has been replaced by myst-parser, which bundles docutils and parses Markdown tables natively, removing the need for two extensions and a link workaround.

This PR is opened as a draft because of the following issues that are currently left to handle:

  • Fix titles hierarchy. Inconsistent headers hierarchy becomes a warning (treated as error in CI) with the new parser.
  • Fix internal links. This is most probably a consequence of the workaround removed in 71b6cfd, that now needs correcting links.
  • Bring back treating warnings as error (i.e. revert ef40d47). I deactivated this in order to list all issues in CI.
  • Check if it is necessary to update toc_tree syntax.

All warnings from latest run are listed below.


source/simulate/replicate-simulation-inputs.md:54: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
source/simulate/replicate-simulation-inputs.md:87: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
source/simulate/replicate-simulation-inputs.md:138: WARNING: Non-consecutive header level increase; H1 to H3 [myst.header]
checking consistency... source/coding-the-legislation/10_basic_example.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/20_input_variables.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/25_vectorial_computing.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/35_periods.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/40_legislation_evolutions.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/50_entities.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/bootstrapping_a_new_country_package.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/inferences.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/legislation_parameters.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/reforms.md: WARNING: document isn't included in any toctree
source/coding-the-legislation/writing_yaml_tests.md: WARNING: document isn't included in any toctree
source/contribute/commit-messages.md: WARNING: document isn't included in any toctree
source/contribute/developer-guide.md: WARNING: document isn't included in any toctree
source/contribute/documentation.md: WARNING: document isn't included in any toctree
source/contribute/extensions.md: WARNING: document isn't included in any toctree
source/contribute/guidelines.md: WARNING: document isn't included in any toctree
source/contribute/language.md: WARNING: document isn't included in any toctree
source/contribute/release-process.md: WARNING: document isn't included in any toctree
source/contribute/semver.md: WARNING: document isn't included in any toctree
source/contribute/tests.md: WARNING: document isn't included in any toctree
source/contribute/variables-naming.md: WARNING: document isn't included in any toctree
source/installation/howto-web-no-local-install.md: WARNING: document isn't included in any toctree
source/installation/howto_docker.md: WARNING: document isn't included in any toctree
source/installation/offline-environment.md: WARNING: document isn't included in any toctree
source/installation/test_situations.md: WARNING: document isn't included in any toctree
source/installation/windows-no-admin.md: WARNING: document isn't included in any toctree
source/key-concepts/parameters.md: WARNING: document isn't included in any toctree
source/key-concepts/periodsinstants.md: WARNING: document isn't included in any toctree
source/key-concepts/person,_entities,_role.md: WARNING: document isn't included in any toctree
source/key-concepts/reforms.md: WARNING: document isn't included in any toctree
source/key-concepts/simulation.md: WARNING: document isn't included in any toctree
source/key-concepts/tax_and_benefit_system.md: WARNING: document isn't included in any toctree
source/key-concepts/variables.md: WARNING: document isn't included in any toctree
source/openfisca-python-api/commons.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/entities.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/enum_array.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/errors.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/holder.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/openfisca_serve.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/openfisca_test.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/parameters.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/periods.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/populations.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/reforms.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/simulation_generator.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/simulations.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/tax-benefit-system.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/test_runner.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/tracer.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/types.rst: WARNING: document isn't included in any toctree
source/openfisca-python-api/variables.rst: WARNING: document isn't included in any toctree
source/openfisca-web-api/config-openapi.md: WARNING: document isn't included in any toctree
source/openfisca-web-api/endpoints.md: WARNING: document isn't included in any toctree
source/openfisca-web-api/input-output-data.md: WARNING: document isn't included in any toctree
source/openfisca-web-api/trace-simulation.md: WARNING: document isn't included in any toctree
source/simulate/analyse-simulation.md: WARNING: document isn't included in any toctree
source/simulate/profile-simulation.md: WARNING: document isn't included in any toctree
source/simulate/replicate-simulation-inputs.md: WARNING: document isn't included in any toctree
source/simulate/run-simulation.md: WARNING: document isn't included in any toctree

@MattiSG MattiSG marked this pull request as draft May 10, 2022 16:25
@MattiSG
Copy link
Member Author

MattiSG commented May 25, 2022

Yes, it is necessary to update the toc_tree syntax 😉
Capture d’écran 2022-05-25 à 17 30 43

@MattiSG
Copy link
Member Author

MattiSG commented May 25, 2022

The only blocking point left is ambiguous references in the docstrings generated from Core, which can only be fixed there. I opened openfisca/openfisca-core#1128. Once it is merged, jobs here should be rerun.

@MattiSG MattiSG marked this pull request as ready for review May 25, 2022 16:34
@MattiSG MattiSG requested review from sandcha and bonjourmauko May 25, 2022 16:34
@MattiSG MattiSG linked an issue May 25, 2022 that may be closed by this pull request
@bonjourmauko
Copy link
Member

I'm still getting an error, the same than in CI:

(openfisca-doc-3.8.13) openfisca-doc-3 λ › make test                                                                                                                    openfisca/openfisca-doc update-deps

Warning, treated as error:
/Users/hyperion/.pyenv/versions/3.8.13/envs/openfisca-doc-3.8.13/src/openfisca-core/openfisca_core/commons/__init__.py:docstring of openfisca_core.commons.formulas.apply_thresholds::py:class reference target not found: nptyping.types._ndarray.NDArray
make[1]: *** [dummy] Error 2
make: *** [test] Error 2

@MattiSG
Copy link
Member Author

MattiSG commented May 30, 2022

Thanks @maukoquiroga! Indeed, this error appeared after the other ones were fixed by openfisca/openfisca-core#1128. It seems to have been “shadowed” somehow by the previous ones.
Unfortunately, I cannot reproduce locally, which makes it very hard to fix. I intended to work on a branch of Core and iterate with CI.
However, if you can reproduce, and since you understand docstrings much better than I do, I would be very happy for your contribution on fixing docstrings in Core! Alternatively, we can maybe have a short call so you can show me how you reproduce that locally. Thanks!

@bonjourmauko
Copy link
Member

bonjourmauko commented May 30, 2022

@MattiSG This problem was being ignored, that's all.

So either:

  1. Continue ignoring it / somehow
  2. Fixing it / following https://github.com/ramonhagenaars/nptyping/blob/master/USERDOCS.md

To reproduce I just did a clean installation :

pyenv virtualenv 3.8.13 blabla-3.8.13
make install
make test

Be sure to keep your venv isolated from others.

I can't fix it now as it demands more cognitive effort that I can afford after ~ 12h of train, but I could yes.

@bonjourmauko
Copy link
Member

Well it looks not just nptyping but all of the actual type aliases, I'm wondering it are not missing a config or plugin?

@bonjourmauko
Copy link
Member

@MattiSG It looks better now: it seems to me we need to fix outdated docstrings and use either Google or Numpy style —currently our styleguide recommends Google style. Can you reproduce now?

(openfisca-doc-3.8.13) openfisca-doc-3 λ › make test                                                                                                                    openfisca/openfisca-doc update-deps

Warning, treated as error:
/xxx/.pyenv/versions/3.8.13/envs/xxx/src/openfisca-core/openfisca_core/parameters/parameter.py:docstring of openfisca_core.parameters.parameter.Parameter::py:class reference target not found: string
make[1]: *** [dummy] Error 2
make: *** [test] Error 2
(openfisca-doc-3.8.13) openfisca-doc-3 λ ›       

@MattiSG
Copy link
Member Author

MattiSG commented Jun 16, 2022

Thanks @maukoquiroga for your commit and your branch in openfisca/openfisca-core#1131! This does indeed fix most of the issues. I just made one more pass and the only warning left at this stage is a very mysterious <unknown>:1: WARNING: py:obj reference target not found: openfisca_core.types.data_types.arrays.T.

As can be seen from the message, there is no reference whatsoever to any file or module, making this error basically impossible to fix. After a lot of research, I tried the following options, to no avail:

  1. Search for openfisca_core.types.data_types.arrays, T, py:obj and variants in the whole codebase.
  2. Bump Python to 3.9.
  3. Set the nitpick_ignore option in 1f98c04.
  4. Set nitpick = False in dc25bf7 (even though this should be the default).

At this stage, I am at a complete loss as to how to fix this warning. The only way forward I see left for us is to stop treating warnings as errors and proceed, so we can finally have the doc build again and be done with this work that has been ongoing for over a month now. However, that would mean losing feedback on docstrings issues when building the doc.

@MattiSG
Copy link
Member Author

MattiSG commented Jun 16, 2022

Okay, we can fix this by removing the nitpicky option, which was actually triggered by the command-line, as demonstrated in 34bde42. This means that broken references will not warn anymore, but that will enable us to proceed. Most of the work needed to pass was about these broken links, and I am not entirely certain of their value to the end user anyway.

@MattiSG
Copy link
Member Author

MattiSG commented Jun 16, 2022

This branch should now pass all tests when openfisca/openfisca-core#1131, which fixes docstrings in Core, is merged.

Unfortunately, openfisca/openfisca-core#1131 is not ready for review since there are additional type linting errors introduced by it in Core.

MattiSG and others added 3 commits June 17, 2022 11:36
Switch off “nitpicky” Sphinx option
Fix otherwise impossible to fix “py:obj reference target not found” error
@MattiSG
Copy link
Member Author

MattiSG commented Jun 17, 2022

All errors in openfisca/openfisca-core#1131 are fixed. This PR is ready for review, and the build should pass once the PR in Core is merged.

Once this is merged, the update-deps-collect-info branch should be deleted. It was there to document experimentations to pass CI.

@MattiSG MattiSG requested review from sandcha and bonjourmauko and removed request for bonjourmauko and sandcha June 17, 2022 09:40
@MattiSG
Copy link
Member Author

MattiSG commented Jun 17, 2022

I triggered a new build now that openfisca/openfisca-core#1131 has been merged. As planned, the build now passes 🎉 This PR is just waiting for an approval to get documentation compilation to work again, which will then enable merging all content improvements that have been waiting for a while.

poke @openfisca/international-maintainers

@MattiSG MattiSG mentioned this pull request Jun 17, 2022
@MattiSG MattiSG merged commit e3a437f into master Jun 17, 2022
@MattiSG MattiSG deleted the update-deps branch June 17, 2022 14:47
MattiSG added a commit to openfisca/openfisca-core that referenced this pull request Jul 7, 2022
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.

Update dependencies
3 participants