openfisca / openfisca-core

OpenFisca core engine. See other repositories for countries-specific code & data.
https://openfisca.org
GNU Affero General Public License v3.0
168 stars 75 forks source link

Fix doc test & build dependency errors #1141

Closed bonjourmauko closed 2 years ago

bonjourmauko commented 2 years ago

Fixes #1136 Fixes #1140

Technical changes

Note: this PR is the continuation of #1137, but seems the solution was not complete. This PR should unblock doc tests failing because of dependency mismatch.

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 78.94% when pulling 3a026fb75b1ae7fd025bbe80bbd4e816ef7bc8a9 on fix-pristine-editable-install into e18f23be34b5964e89ee65e33669da1a0996ee73 on master.

MattiSG commented 2 years ago

As described in https://github.com/openfisca/openfisca-core/issues/1140#issuecomment-1200902579, I believe there is no way we currently can reproduce the documented failure, so we cannot know if publishing this version will fix the problem. I was trustful for #1137, but I cannot approve this PR under these circumstances now, especially considering that it currently reverts #1137 yet introduces other untestable dependency changes.

I would fast approve a pure #1137 revert, considering that the issue #1136 it fixed was extremely minor (misleading log with no consequence), or could consider approving this if #1140 included a repeatable procedure for testing changes. I would also ask that the 1> /dev/null silencers in the test-doc-install step are removed so we can follow better what is happening.

bonjourmauko commented 2 years ago

I would like us to understand the reason why the tests passed in #1137 and failed afterwards on master. What differs? Until we understand this, all fixes are bound to be uncertain. We cannot iterate on production versions by publishing a new patch every day.

Yes, it's a revert / and could also be handled as such TBH. I gave some hours to try to understand the issue, and gave up by reverting that specific lib upgrade after taking a closer look at the errors.

IMHO the ideal way of being able to catch this sooner is to include doc dependencies in in openfisca-core (for example .[doc]).

It's a trade-off, certainly.

bonjourmauko commented 2 years ago

As described in #1140 (comment), I believe there is no way we currently can reproduce the documented failure, so we cannot know if publishing this version will fix the problem. I was trustful for #1137, but I cannot approve this PR under these circumstances now, especially considering that it currently reverts #1137 yet introduces other untestable dependency changes.

I would fast approve a pure #1137 revert, considering that the issue #1136 it fixed was extremely minor (misleading log with no consequence), or could consider approving this if #1140 included a repeatable procedure for testing changes. I would also ask that the 1> /dev/null silencers in the test-doc-install step are removed so we can follow better what is happening.

Actually the flake8 upgrade is a knight unrelated to the fix.

bonjourmauko commented 2 years ago

Fantastic, so what I get now:

flake8.exceptions.FailedToLoadPlugin: Flake8 failed to load plugin "pycodestyle.missing_whitespace_after_import_keyword" due to module 'pycodestyle' has no attribute 'missing_whitespace_after_import_keyword'.

So this is a layer 8 error: instead of downgrading pycodestyle, I proposed upgrading flask8.

It concurred with the silencing and we got no clue of what was happing.

Let's remove the silencers!

bonjourmauko commented 2 years ago

Now by suppressing the silencers we have actionable logs:

The conflict is caused by:
    sphinx 4.5.0 depends on importlib-metadata>=4.4; python_version < "3.10"
    flake8 4.0.1 depends on importlib-metadata<4.3; python_version < "3.8"
    sphinx 4.5.0 depends on importlib-metadata>=4.4; python_version < "3.10"
    flake8 4.0.0 depends on importlib-metadata<4.3; python_version < "3.8"

https://github.com/openfisca/openfisca-core/runs/7611320470?check_suite_focus=true

Which confirms the need to rollback that upgrade.

Rollback or no,

Or could consider approving this if https://github.com/openfisca/openfisca-core/issues/1140 included a repeatable procedure for testing changes. I would also ask that the 1> /dev/null silencers in the test-doc-install step are removed so we can follow better what is happening.

I think this PR now provides an increase in our understanding of what's going on.

bonjourmauko commented 2 years ago

As I put it in the last commit:

Versions >= 4.0.0 of flake8 require a library version, for clients using Python < 3,8, that is incompatible with the same library requirement by Sphinx >= 4.5.