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

build: test built version #54

Merged
merged 46 commits into from
Oct 18, 2024
Merged

build: test built version #54

merged 46 commits into from
Oct 18, 2024

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Dec 21, 2022

Part of openfisca/openfisca-core#1064
Depends on openfisca/country-template#159

  • Technical Changes
  • Details:
    • Run tests in absolute isolation
      • Uses poetry to ensure deterministic builds
      • Uses tox to test builds in isolation

@bonjourmauko bonjourmauko changed the title WIP Ensure deterministic releases Dec 21, 2022
@bonjourmauko bonjourmauko marked this pull request as ready for review December 21, 2022 12:40
@bonjourmauko bonjourmauko requested a review from a team October 16, 2024 12:08
@bonjourmauko
Copy link
Member Author

I approve the switch to Poetry 👍

Could you take a look now?

Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really qualified for this PR.
I just noticed something weird.
You may need @benoit-cty or @sandcha reviewing it.
Might also be nice if changes made here are propagated to france (or vice versa) to keep the tooling in line.

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Oct 16, 2024

Not really qualified for this PR. I just noticed something weird.

What is weird?

You may need @benoit-cty or @sandcha reviewing it. Might also be nice if changes made here are propagated to france (or vice versa) to keep the tooling in line.

I'm trying (since 2021) to equalise tooling of core/country/extension.

Happy to help with France, it comes right after in the pipe.

@benjello
Copy link
Member

Not really qualified for this PR. I just noticed something weird.

What is weird?

The yamlint addition ----

@benjello
Copy link
Member

You may need @benoit-cty or @sandcha reviewing it. Might also be nice if changes made here are propagated to france (or vice versa) to keep the tooling in line.

I'm trying (since 2021) to equalise tooling of core/country/extension.

Happy to help with France, it comes right after in the pipe.

I will make my point clearer : @benoit-cty and @sandcha worked a lot on the tooling in france (and I don't know if ti was portted to country-template and extensions) so I just warned about divergences and double work.

@bonjourmauko
Copy link
Member Author

Not really qualified for this PR. I just noticed something weird.

What is weird?

The yamlint addition ----

Ah. I did add the yaml check as it is run in country-template. Maybe do we have a yamllint config in core or france?

.yamllint Show resolved Hide resolved
@benoit-cty
Copy link
Contributor

In OpenFisca-France, we did have this config in .yamlint :

---
extends: relaxed

rules:
  truthy: enable
  # Disable warning level rules to remove flood in yamllint output
  # YAML file should be modified to comply to those rules before activation
  indentation: disable
  line-length: disable

But I can't say much about it, I never work on it.

@benoit-cty
Copy link
Contributor

I approve the switch to Poetry 👍

Could you take a look now?

My comment where from 2 years ago.
Did the switch to Poetry been discussed by the core team ?
My advice now is that OpenFisca will better use something like https://hatch.pypa.io/latest/ (I use it on another project) or https://github.com/astral-sh/uv (I have never used it)

Why ?
Because Hatch manage your env and Python version. So it replace make, pip, brew, venv...

That being said, we are using Poetry in all our Python project at LexImpact so it's easier to get support than on Hatch or UV...

@bonjourmauko
Copy link
Member Author

bonjourmauko commented Oct 17, 2024

Sorry I didn't properly reply in my earlier comment. I'll do below:

Did the switch to Poetry been discussed by the core team ?

There has been scattered discussions mainly in core.

The last general discussion on roadmap is this one: openfisca/openfisca-core#942

The last council decision (RFC) on tooling is this one: openfisca/openfisca-core#1040 (comment)

And the general problem this PR aims to fix is about properly testing against built versions openfisca/openfisca-core#1064

Poetry here is then, at least for me, accessory to the fact of being able to easily have a build/deploy flow that allows for openfisca libraries (akin to waht I've been doing with rattler/mamba for conda).

In particular, this PR relies on the virtuous interaction of tox and poetry together to achieve the objective of having deterministic releases whose builds are systematically tested.

I could still achieve the same result, although more laboriously —which we want to avoid, as these things have to be maintained over time— without poetry.

So, it that is a blocker, I can remove it (as, again, at least here, is only accessory).

My advice now is that OpenFisca will better use something like https://hatch.pypa.io/latest/ (I use it on another project) or https://github.com/astral-sh/uv (I have never used it)

I have not knowledge on any of these tools. However, I'm personally agnostic on the tooling given that:

Why ? Because Hatch manage your env and Python version. So it replace make, pip, brew, venv...

If we adopt a tool replacing make (which is ok, as seen in the RFC), the big feature it has to have is to allow for easy parallelisation of tasks.

As poetry nor tox are here used as a mean to replace make as the de facto tool for contributors, I sort of feel that hatch and uv are at least here meant for differents use cases, so they're not in competition.

That being said, we are using Poetry in all our Python project at LexImpact so it's easier to get support than on Hatch or UV...

And that is a good point. So long as any tool uses a more or less standard pyproject.toml, adopting one tool or the other, at least for the use case of this PR, is an easily revertable or revokable decision.

All that being said, I'd happily take a look at hatch or uv for the implementation of the RFC. For the subject matter of these changes, however (deterministic and tested builds) they seem a bit overkill to me, IMHO.

Thoughts?

Copy link
Contributor

@benoit-cty benoit-cty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I install with make install, then do make format I end up with:
/bin/sh: 1: black: not found
I think it's a blocker but maybe I'm missing a command ?

I suggest to put poetry run in front of all command : dad766d

Why don't use poetry env remove --all in make uninstall instead of pip ?

Makefile Outdated Show resolved Hide resolved
@benoit-cty
Copy link
Contributor

Did the switch to Poetry been discussed by the core team ?
[...]

Thanks for the details !

As you say, using Poetry is a nice step. It doesn't prevent using another tool in the future. It's not blocking at all 👍

If you are curious about Hatch, I use it for https://github.com/mlco2/codecarbon/blob/master/CONTRIBUTING.md#some-hatch-commands where it replace venv, poetry, make and tox. But it can be a huge step for the OpenFisca community.

@bonjourmauko
Copy link
Member Author

If I install with make install, then do make format I end up with: /bin/sh: 1: black: not found I think it's a blocker but maybe I'm missing a command ?

I suggest to put poetry run in front of all command : dad766d

Why don't use poetry env remove --all in make uninstall instead of pip ?

I'll try that.

Makefile Show resolved Hide resolved
@bonjourmauko
Copy link
Member Author

Ping @benoit-cty

@bonjourmauko
Copy link
Member Author

Did the switch to Poetry been discussed by the core team ?
[...]

Thanks for the details !

As you say, using Poetry is a nice step. It doesn't prevent using another tool in the future. It's not blocking at all 👍

Poetry and tox solve rather effortlessly a long-lasting problem we have. I try to prioritise maintainability.

And due to some overly-clever ways that core handle things, I always fear that big changes will break things.

If you are curious about Hatch, I use it for https://github.com/mlco2/codecarbon/blob/master/CONTRIBUTING.md#some-hatch-commands where it replace venv, poetry, make and tox. But it can be a huge step for the OpenFisca community.

Thanks!

That being said, the builder protocol is now standard, so we can perfectly mix together poetry and hatch.

In another project, where I have to compile some extensions in C and C++, I use poetry + mason, for example.

Copy link
Contributor

@benoit-cty benoit-cty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

@bonjourmauko bonjourmauko merged commit e232d82 into master Oct 18, 2024
7 checks passed
@bonjourmauko bonjourmauko deleted the add-poetry branch October 18, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:build Changes that affect the build system or external dependencies
Projects
Development

Successfully merging this pull request may close these issues.

3 participants