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

CEP: support for ABI3 packages #86

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

CEP: support for ABI3 packages #86

wants to merge 19 commits into from

Conversation

isuruf
Copy link

@isuruf isuruf commented Jul 1, 2024

Description

@isuruf isuruf marked this pull request as draft July 1, 2024 17:03
cep-abi3.md Outdated Show resolved Hide resolved
cep-abi3.md Outdated Show resolved Hide resolved
isuruf and others added 2 commits July 1, 2024 17:20
Co-authored-by: Pavel Zwerschke <pavelzw@gmail.com>
cep-abi3.md Outdated

In particular an option:
```
build:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for V1 recipes we would use:

build:
  python:
    version_independent: true/false

cep-abi3.md Outdated
In particular an option:
```
build:
python_version_independent: true
Copy link

Choose a reason for hiding this comment

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

Is version_independent better than abi3? It seems possibly misleading to me because a package can still set a minimum Python version and so be Python version-dependent.

Copy link
Author

Choose a reason for hiding this comment

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

abi3 packages with package_metadata_version = 2 should not be that different from any other package. It just has more things in info/link.json.

Copy link

@mariusvniekerk mariusvniekerk Jul 3, 2024

Choose a reason for hiding this comment

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

i think something like

python:
   abi: abi3

leaves open enough space to support future expansion of other abis that may require special behavior

for example

python:
   abi: hpy

Copy link
Author

Choose a reason for hiding this comment

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

@mariusvniekerk, both those options need the exact same thing. (B1-B4)

Copy link
Author

Choose a reason for hiding this comment

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

Here's what a conda-build PR would look like conda/conda-build#5456

Choose a reason for hiding this comment

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

Note that it's fairly likely that abi4 will become a thing in 2025 or 2026, since it seems necessary for free-threading to be able to support the Limited C API. python-abi3 in host requirements seems to cover it though.

cep-abi3.md Outdated
currently. This requires support from build tools to set `subdir: <platform>`
only.

In particular an option:
Copy link

Choose a reason for hiding this comment

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

Is it planned that the conda-forge tooling would check for this option and then set up Windows/Mac/Linux builds even though noarch: python is specified where it would otherwise only build on one arch?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can do that.

cep-abi3.md Outdated
that the package was built for.

&nbsp;&nbsp;<strong>C2</strong>:
They have `noarch: python`.
Copy link

Choose a reason for hiding this comment

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

Should there be a suggestion for a new way to specify abi3 packages in this proposal? I understand how noarch: python almost works already and so can be used to get abi3 packages working with existing tools, but it is a misleading label since these packages are still architecture dependent.

Also, I had tried to test with post-link.sh and pre-unlink.sh scripts previously here but I didn't try noarch: python and got stuck on the run exports of Python. Maybe I just didn't know the right syntax, but it seemed like when you don't use noarch: python conda-build pins the Python version in a way that is hard to override by setting the Python version in the recipe requirements. That might be worth mentioning as another important behavior of noarch: python (if I was not missing something).

Copy link
Author

Choose a reason for hiding this comment

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

Should there be a suggestion for a new way to specify abi3 packages in this proposal? I understand how noarch: python almost works already and so can be used to get abi3 packages working with existing tools, but it is a misleading label since these packages are still architecture dependent.

This is package_metadata_version = 1 which is for existing tool compatibility. Please see the next section for new ways.

Also, I had tried to test with post-link.sh and pre-unlink.sh scripts previously conda-forge/qiskit-terra-feedstock#41 but I didn't try noarch: python and got stuck on the run exports of Python. Maybe I just didn't know the right syntax, but it seemed like when you don't use noarch: python conda-build pins the Python version in a way that is hard to override by setting the Python version in the recipe requirements. That might be worth mentioning as another important behavior of noarch: python (if I was not missing something).

That should be fixable by using ignore_run_exports. This is only specific to conda-forge which has run_exports on python package and defaults does not AFAIK. I wanted to keep the CEP more generic.

Comment on lines +153 to +155
Micromamba:
1. Actions `B1, B2, B3` are applied for packages with both `A2, A3`.
2. Action `B4` is applied for packages with both `A2, A4`.
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, rattler follows the same behavior as micromamba.

Copy link
Member

@jezdez jezdez Oct 4, 2024

Choose a reason for hiding this comment

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

I'm interested what your suggestion is for this mismatch in behavior, should conda be adapted to follow mamba and rattler in this case?

Copy link
Author

Choose a reason for hiding this comment

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

My suggestion is to keep the existing mismatched behaviour in package_metadata_version = 1 and make this uniform in package_metadata_version = 2

cep-abi3.md Outdated
Comment on lines 227 to 229
Note that we do not require `B1` as package authors should depend on a python
version that has a custom `site.py` that adds `lib/python/site-packages` to
the path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesnt this imply that older python versions available on for instance conda-forge will never have the ability to use abi3 packages? If we are modifying the installer we could just as well move the Python files to the python specific site-packages directory.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, but we can also have the site.py in a separate conda package as well. I'm trying to reduce all these special things that a installer has to do. With the files in lib/python/site-packages, the only thing that a installer has to do for an abi3 package is to compile the pyc files which is an optional thing anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

But relying on a special package might also be problematic if you are not using conda-forge though.

Copy link
Author

Choose a reason for hiding this comment

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

I'm hoping we can add support in defaults too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not prefer this because it means the proper functioning of the installer is dependent on a specific package. I'd rather encode the behavior in the installer itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the installer should do less special case stuff. But I also don't think that it should depend on specific packages to function properly.

Even with this proposal the installer does already have special case behavior. However, without the dependency on a package that introduces a site.py file it will leave the package in a broken state. To me that adds complexity for the package maintainer.

However, one could argue that the same holds for depending on python itself..

It would be ideal if a recipe author doesn't have to care about these details.

Copy link
Author

Choose a reason for hiding this comment

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

We could make the build tool add this package, then the installer does not have to care about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we do this for other packages? Because then we tie the build-tool to specific package names which might also not be ideal.

Copy link
Author

Choose a reason for hiding this comment

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

We take the C compiler name from c_compiler variable in the config. We could do the same.

Copy link
Author

@isuruf isuruf Jul 12, 2024

Choose a reason for hiding this comment

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

Oh I forgot about run_exports: noarch. We can extend that in the python recipe.

cep-abi3.md Outdated

To test that an install tool works with this scheme, you can try

conda/micromamba create -n a sympy python=3.10 -c isuruf/label/abi3 -c conda-forge
Copy link
Member

Choose a reason for hiding this comment

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

Should we hard-code this into the CEP which may prove to not work anymore at some point?

Copy link
Author

Choose a reason for hiding this comment

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

No, I'll remove it

@isuruf isuruf marked this pull request as ready for review November 21, 2024 20:21
@jaimergp
Copy link
Contributor

@conda/steering-council - @isuruf would like to call a vote on this soon. I'll be championing this request. In the last community call, we decided to give another two weeks for comments, and then call a vote starting December the 4th.

So, if you'd like to make any CEP-altering comments, this is your chance. Once the vote starts, we should only make editorial corrections. Thanks!

This is explicitly required from recipe authors so that we do not
restrict this CEP to ABI3 packages and allow the possibility for ABI4 etc.

An example `python-abi3=3.8` package would set itself in its

Choose a reason for hiding this comment

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

Minor: perhaps this should use the same {{ python_min }} syntax as was recently introduced for noarch packages?

Copy link
Author

Choose a reason for hiding this comment

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

Unlike python which needs multiple values for arch builds and a single value for noarch builds, we only need a single value for abi3 builds. So we can set

python_abi3:
  - 3.8

in our global pinning file in conda-forge and anaconda.

Choose a reason for hiding this comment

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

Okay, that makes sense. Some of the "automatically bump" rationale of python_min probably still applies, but this dependency is indeed easier to handle than python.

Choose a reason for hiding this comment

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

we only need a single value for abi3 builds.

That's not guaranteed AFAIU. Despite the ABI being stable, new features get added in newer python versions, and if anything depends on those features, you might need both the oldest and a newer abi3 build.

For example, cryptography builds for both the py37 & py39 limited API. In the future, it's quite likely that other features (e.g. nogil) will similarly require a newer baseline for some features.

Of course, individual feedstocks will be able to override python_abi3 where necessary (and we may get away to only have a single value in the global pinning), but conceptually, I don't think we can assume it'll be one value.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, but what I want to emphasize is that we don't need different requirements based on arch vs noarch.

@wolfv
Copy link
Contributor

wolfv commented Nov 26, 2024

I just re-read the CEP and it looks great to me. Happy that we can achieve that with the existing metadata.

cep-abi3.md Outdated
<tr><td> Title </td><td> Support for abi3 python packages </td>
<tr><td> Status </td><td> Draft </td></tr>
<tr><td> Author(s) </td><td> Isuru Fernando &lt;ifernando@quansight.com&gt;</td></tr>
<tr><td> Created </td><td> July 01, 2023</td></tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<tr><td> Created </td><td> July 01, 2023</td></tr>
<tr><td> Created </td><td> July 01, 2024</td></tr>
<tr><td> Updated </td><td> Nov 26, 2024</td></tr>
<tr><td> Discussion </td><td> https://github.com/conda/ceps/pull/86 </td></tr>
<tr><td> Implementation </td><td> https://github.com/conda/conda-build/pull/5456 </td></tr>

cep-abi3.md Outdated Show resolved Hide resolved
@jaimergp
Copy link
Contributor

The proposal looks fine! I like how easy is to follow attributes, behaviors and actions with the A1, B2, C3 approach. 👏

I'm echoing @wshanks thoughts here too, though: it feels weird to use a key named noarch to signal something that is indeed arch-specific. I understand we are leveraging the "version-independence" of noarch: python for this, and it's nice that we can get there with arguably little changes, but it still feels odd to keep adding unintuitive semantics to the pile. That said, I don't think it should block this PR.

Also, I'd like to make some editorial changes that don't convey meaning but I'll send those in a separate PR to your branch so this one doesn't get too noisy.

cep-abi3.md Outdated Show resolved Hide resolved
cep-abi3.md Outdated Show resolved Hide resolved
cep-abi3.md Outdated Show resolved Hide resolved
@jaimergp
Copy link
Contributor

jaimergp commented Dec 4, 2024

Hello @conda/steering-council. Requesting a vote for this CEP.

This vote falls under the "Enhancement Proposal Approval" policy of the conda governance policy,
please vote and/or comment on this proposal at your earliest convenience.

It needs 60% of the Steering Council to vote yes to pass.

To vote, please use the following form to vote.

This vote will end on 2024-12-18, End of Day, Anywhere on Earth (AoE).

@xhochy (Uwe Korn)

  • yes
  • no
  • abstain

@CJ-Wright (Christopher J. 'CJ' Wright)

  • yes
  • no
  • abstain

@mariusvniekerk (Marius van Niekerk)

  • yes
  • no
  • abstain

@chenghlee (Cheng H. Lee)

  • yes
  • no
  • abstain

@ocefpaf (Filipe Fernandes)

  • yes
  • no
  • abstain

@marcelotrevisani (Marcelo Duarte Trevisani)

  • yes
  • no
  • abstain

@msarahan (Michael Sarahan)

  • yes
  • no
  • abstain

@mbargull (Marcel Bargull)

  • yes
  • no
  • abstain

@jakirkham (John Kirkham)

  • yes
  • no
  • abstain

@jezdez (Jannis Leidel)

  • yes
  • no
  • abstain

@wolfv (Wolf Vollprecht)

  • yes
  • no
  • abstain

@jaimergp (Jaime Rodríguez-Guerra)

  • yes
  • no
  • abstain

@kkraus14 (Keith Kraus)

  • yes
  • no
  • abstain

@baszalmstra (Bas Zalmstra)

  • yes
  • no
  • abstain

@beckermr (Matthew R. Becker)

  • yes
  • no
  • abstain

@Hind-M (Hind Montassif)

  • yes
  • no
  • abstain

@trallard (Tania Allard)

  • yes
  • no
  • abstain

@jaimergp jaimergp added the vote Voting following governance policy label Dec 4, 2024
They have `noarch: python`.

- **C3**:
`A2, A3, A4` are applied.
Copy link

Choose a reason for hiding this comment

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

Minor: A2 is already covered by C2 if I'm not mistaken. Do I get this right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vote Voting following governance policy
Projects
None yet
Development

Successfully merging this pull request may close these issues.