Add a debug build of python 3.11

nikhilweee commented 2 years ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[x] Bumped the build number (if the version is unchanged)
[x] Reset the build number to 0 (if the version changed)
[x] Re-rendered with the latest conda-smithy (Use the phrase code>@<space/conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
[x] Ensured the license file is being packaged.

There have been multiple conversations around supporting a debug build of python both in this repository (#73) and elsewhere (https://github.com/conda-forge/staged-recipes/issues/4812). There have also been previous attempts at this (#86) but there hasn't been any progress in the last 6 years. This PR is an attempt to revive those efforts.

Fixes https://github.com/conda-forge/python-feedstock/issues/594 Fixes https://github.com/conda-forge/python-feedstock/issues/73 Closes https://github.com/conda-forge/python-feedstock/pull/86 Fixes https://github.com/conda-forge/staged-recipes/issues/1593 Fixes https://github.com/conda-forge/conda-forge.github.io/issues/1017

Closes #598 #599 #600

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@conda-forge-admin, please ping team

Hello team, I'm new to building packages for conda-forge but I'm really interested in a debug build of python. I spent the last couple days trying to figure out a way to include a debug build of python. Initially, in #596 I tried adding a separate output called python-debug but that didn't go so well because compiling python-debug would overwrite the build files for python. I couldn't find a way to use separate build folders so I gathered that it's a bad idea to build both packages simultaneously.

In this PR, I basically changed the name of the output to python-debug and made sure that --with-pydebug is being passed on to configure. All goes well until conda runs tests. As you can see in the reference below, conda-build explicitly adds python as a dependency which causes all sorts of clobber warnings in the CI builds. Is there a way to say that python-debug can basically be substituted for python and there's no need to install the latter? Or is it a bad idea to change the name of the package in the first place?

I am starting to run out of ideas and would really appreciate some help. Thanks!

https://github.com/conda/conda-build/blob/cc6c050ee3325e6957dbf60f74dbe79d78d26a8f/conda_build/metadata.py#L2365-L2369

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/python and so here I am doing that.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/python and so here I am doing that.

nikhilweee commented 2 years ago

Apologies for the double mention, didn't know that the bot will retrigger on edits.

jakirkham commented 2 years ago

We may want to recheck this Windows ABI incompatibility issue that Ray had flagged ( https://github.com/conda-forge/python-feedstock/issues/73#issuecomment-605151779 )

nikhilweee commented 2 years ago

Okay so after the last comment I figured that I shouldn't change the package name because that's how conda figures out not to install another version of python if this debug version is already installed. However, I did change the build string to include dbg. I believe now people can install a debug version using conda install python=*=*dbg* -c conda-forge

@isuruf I didn't intend to merge this into main because that would mean overwriting the non-debug version. I was under the impression that maybe we could maintain {VER}-dbg branches separately and update them less often but I'm open to other ideas. On another note, after almost a week of struggling, the linux builds are now passing and I feel good about my first PR.

jakirkham commented 2 years ago

Think it would be better to build this with the main branch and just add extra jobs or iterate over builds in a loop (whichever is easiest). If we stick this in a different branch, it will likely fall out of date and become incompatible with the normal python build. Additionally sticking this in main makes it easier to backport this change to older python versions.

nikhilweee commented 2 years ago

In that case I should add the flags used to differentiate between the two builds. I had removed it earlier.

jakirkham commented 2 years ago

We could add a variant to conda_build_config.yaml perhaps like so. Once incorporated into the recipe (and re-rendered), this should generate the additional jobs

nikhilweee commented 2 years ago

Thanks @jakirkham, that was very helpful. I added two variants in conda_build_config.yaml. Let's see how the tests go.

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@mbargull Can you point me in the right direction on how to push to a separate label? I guess I'll need to pass on the --label argument to conda mambabuild but I can't edit build_steps.sh since that's overwritten by conda-smithy.

nikhilweee commented 2 years ago

Sorry I didn't intend to mass-remove reviewers and now I don't see a way to add them back. Apologies.

nikhilweee commented 2 years ago

I think at this point all except windows debug builds should be passing. I have no idea how to solve that and neither am I able to fully understand https://github.com/conda-forge/python-feedstock/issues/73#issuecomment-605151779 so I guess I could really use some help.

jakirkham commented 2 years ago

No worries. This seems to still be in progress IIUC. We can add reviewers back when needed

jakirkham commented 2 years ago

I think at this point all except windows debug builds should be passing.

From the Python docs:

sys.abiflags

On POSIX systems where Python was built with the standard configure script, this contains the ABI flags as specified by PEP 3149.

So my guess is we have some test of sys.abiflags somewhere that needs to be skipped on Windows. That said, atm am not finding the test here (though could be overlooking something). These lines appear to be run right before the error (if that provides any clues)

nikhilweee commented 2 years ago

@jakirkham Yes you're right, I added those tests as part of this PR. In fact testing sys.abiflags and sys. gettotalrefcount are the only tests that are unique to the debug build. I'll change them to not run on Windows but then we'll have no way to test whether Windows debug builds are correct in any shape or form.

jakirkham commented 2 years ago

Yes you're right, I added those tests as part of this PR.

🤦‍♂️ Yeah that makes sense. Didn't actually look here 😅 Probably time to call it a night 😴

...but then we'll have no way to test whether Windows debug builds are correct in any shape or form.

Tried searching for an alternative, but was unsuccessful (though also don't have a Windows machine to play with). There probably is an alternative, but it may require more searching or trying on a Windows machine. One can inspect flags Python was built with for example.

mbargull commented 2 years ago

I think at this point all except windows debug builds should be passing. I have no idea how to solve that and neither am I able to fully understand #73 (comment) so I guess I could really use some help.

The preferable thing would of course be to get this to work on Windows too. But if there are indeed concerns about ABI compatibility for that platform, we could also

  skip: True  # [win and build_type == "debug"]

and revisit this later when/if someone needs a debug build on Windows.

mbargull commented 2 years ago

@conda-forge-admin, please rerender

github-actions[bot] commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you but ran into some issues. Please check the output logs of the latest rerendering GutHub actions workflow run for errors. You can also ping conda-forge/core for further assistance or try re-rendering locally.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/python-feedstock/actions/runs/3472523918.

mbargull commented 2 years ago

@conda-forge-admin, please rerender

mbargull commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@jakirkham @mbargull I think at this point all builds are passing. Is this ready to be merged?

mbargull commented 2 years ago

@nikhilweee, thanks for working on this! Let's give others time to review before we proceed.

jakirkham commented 2 years ago

Do we want this in a different label or do we just want some variant to select debug (like we do with PyPy, MPI, BLAS, etc.)?

mbargull commented 2 years ago

Do we want this in a different label or do we just want some variant to select debug (like we do with PyPy, MPI, BLAS, etc.)?

I'm -1 to a variant in the main channel. See https://github.com/conda-forge/python-feedstock/pull/597#pullrequestreview-1180378693 , specifically:

The main cases for track_features are when we want certain variants/packages not installed by default but still have the possibility so seamlessly introduce them even implicitly (e.g. via dependencies). Since we shouldn't have the need for that implicit behavior and track_features is a non-perfect workaround, I would not go for track_features.

For one, it is usually undesirable to install a debug build unless you really want one. But also bare in mind that this isn't doesn't create a simple drop-in variant: the shared objects have another ABI tag, i.e., 311d instead of just 311 (the suffix for extensions would also be cpython-311d-x86_64-linux-gnu.so and similar).

isuruf commented 2 years ago

But also bare in mind that this isn't doesn't create a simple drop-in variant: the shared objects have another ABI tag, i.e., 311d instead of just 311 (the suffix for extensions would also be cpython-311d-x86_64-linux-gnu.so and similar).

That doesn't mean that extensions compiled with a release build cannot be loaded.

mbargull commented 2 years ago

But also bare in mind that this isn't doesn't create a simple drop-in variant: the shared objects have another ABI tag, i.e., 311d instead of just 311 (the suffix for extensions would also be cpython-311d-x86_64-linux-gnu.so and similar).

That doesn't mean that extensions compiled with a release build cannot be loaded.

Right, I only meant the extensions that have been built with the debug version.

jakirkham commented 2 years ago

Our knowledge of how to use features, particularly with Python (PyPy being a great example), has gotten a lot better over the years. Think we could go that route safely if we wanted to

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

mbargull commented 2 years ago

Our knowledge of how to use features, particularly with Python (PyPy being a great example), has gotten a lot better over the years. Think we could go that route safely if we wanted to

It's not about knowledge but (solver) logic. I'm quite familiar how the solver works; my deep dive into it was over 4 years ago and it still works the same. Conda's strategy is to offer a solution if possible even if it isn't a good one. It'll take escape hatches if there are any. We only use track_features because no one proposed/implemented a sound strategy to solve the use cases we (ab-)use track_features now (the original design was different from how we use them now). We don't have to reiterate all of this; but here is a simple demonstration of some of its pitfalls:

(base) conda create -y --name=test >/dev/null 2>&1
(base) conda activate test
(test) # Pretend we'd have some package with a constraint that
(test) #   - prevents installation of python[version=>=3.9.10,build=*_cpython]
(test) #   - allows installations of python[version=>=3.9.10,build=*_pypy]
(test) conda config --env --add pinned_packages 'ld_impl_linux-64<2.36'
(test) # In the following, *_pypy packages are never requested per CLI nor history specs.
(test) # ... we still end up and get stuck ("features" linger per solver logic!) with them anyway...
(test) conda install -qd python=3.9.10 2>&1 | grep 'python-3'
  python             conda-forge/linux-64::python-3.9.10-0_73_pypy None
(test) mamba install -qy python=3.9.10 2>&1 | grep 'python *3'
  + python              3.9.10  0_73_pypy      conda-forge/linux-64     Cached
(test) # Lift the constraint -- *_cpython packages are still not installed by default
(test) # (as per design from the track_features mechanism!)
(test) conda config --env --remove-key pinned_packages
(test) conda install -qd python=3.9.12 2>&1 | grep 'python *3'
  python                                   3.9.10-0_73_pypy --> 3.9.12-0_73_pypy None
(test) mamba install -qd python=3.9.12 2>&1 | grep 'python *3'
  - python    3.9.10  0_73_pypy   conda-forge                    
  + python    3.9.12  0_73_pypy   conda-forge/linux-64     Cached
(test) conda install -qd python=3.9.12=\*cpython 2>&1 | grep 'python *3'
  python                                   3.9.10-0_73_pypy --> 3.9.12-h2660328_1_cpython None
(test) mamba install -qd python=3.9.12=\*cpython 2>&1 | grep 'python-3'
  - package pypy3.9-7.3.8-h986b250_2 has constraint python 3.9.* *_73_pypy conflicting with python-3.9.12-h9a8a25e_0_cpython

We do use track_features to have an easy way for users to install, e.g., PyPy. But it is still a brittle construct -- most of the time it works, but it can also fail easily (as demonstrated above). As stated before, with debug builds we (IMO) don't have the need to rely on implicit, sometimes working behavior, i.e., provide an "easy way for users to install" them, but can use a proper, logically more sound way (i.e., separate channels). With track_features there is always a (albeit small) possibility for regular users to end up with the debug builds; with separate channels there is none. Regular users don't need the debug builds; hence "none" is a better choice here.

nikhilweee commented 2 years ago

@mbargull How long does the review process generally take?

nikhilweee commented 2 years ago

It's also strange that we still have windows debug builds instead of release builds even when recipe.yaml explicitly skips windows debug builds. I feel that this is a side effect of removing Circle CI from the recipe. @jakirkham should I add it back?

nikhilweee commented 2 years ago

Im also curious to know why we ship __pycache__ and *.pyc? This may be a really dumb question but aren't those files regenerated automatically? In my analysis we can further save some space by removing those files.

chrisburr commented 2 years ago

Im also curious to know why we ship pycache and *.pyc? This may be a really dumb question but aren't those files regenerated automatically? In my analysis we can further save some space by removing those files.

This is how it's normally done by package managers (for example see the fedora's Python 3 file list). There are a few reasons (and I'm probably forgetting others):

don't want to be executing Python at install time to regenerate them (you might not even be able to if you're installing from a different platform)
generating on demand might not be possible if the environment isn't writable
generating on demand results in files being left behind when you uninstall packages
adding Python-specific workarounds to the package manager becomes problematic when Python changes as you suddenly have old versions of conda doing the wrong thing

nikhilweee commented 2 years ago

@chrisburr Those are really great points! Thank you for the explanation.

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@conda-forge-admin, please ping team

It's been a while since this PR is open and It would be great to get an opinion from the team before we move forward. If this is left unaddressed I'm afraid this PR will fall behind other commits and I might not be able to keep up with it.

conda-forge-linter commented 2 years ago

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/python and so here I am doing that.

nikhilweee commented 2 years ago

I also wanted to know what's the best way to incorporate upstream changes? Right now I did a rebase but I'm not sure if that's the recommended way.

mbargull commented 2 years ago

I also wanted to know what's the best way to incorporate upstream changes? Right now I did a rebase but I'm not sure if that's the recommended way.

There are multiple ways but no recommended way. Personally, for our feedstocks, esp. the more involved and important ones like this one, I find it easiest for the reviewers to have a somewhat clear change set, i.e., few unrelated changes and separable commits. That can be achieved with rebases/cherry-picking/squashes, if needed -- the downside is that needed force-pushes for that can make previous CI runs and their logs less visible; hence one should make deliberate/sparingly use of those.

In this case, we have the history of different attempts and some combination of merged and rebased commit which leaves us with a long list of commits that takes a bit of time to comprehend. Hence, I'd probably reorder+squash related commits and cherry-pick your changes on top of our main branch to make this now more comprehensible.

mbargull commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

nikhilweee commented 2 years ago

Okay, I'm not very happy about force pushing, but the history should be much cleaner now.

nikhilweee commented 2 years ago

@conda-forge-admin, please rerender

jakirkham commented 2 years ago

Could you please explain why that run_export should be added? Asking as it is my understanding this should be ABI compatibility on UNIX (all that is built now). Also this has been segregated into a different label (where other packages that would build against likely would need to go). That said, not sure that we are going to build packages dependent on the debug build of this package (since we can reuse their existing builds)

conda-forge / python-feedstock

Add a debug build of python 3.11 #597