Release 23.3 - Githubissues

mattwthompson commented 1 year ago

Checklist

[x] Used a personal fork of the feedstock to propose changes
[ ] 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)
[ ] Ensured the license file is being packaged.

conda-forge-webservices[bot] commented 1 year 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.

mattwthompson commented 1 year ago

@conda-forge-admin, please rerender

mattwthompson commented 1 year ago

I cherry-picked the 3.11 file .ci_support/migrations/python311.yaml from #113. I'm not at all confident that'll work; if it doesn't I'll just remove it so we can get this version out.

mikemhenry commented 1 year ago

I cherry-picked the 3.11 file .ci_support/migrations/python311.yaml from #113. I'm not at all confident that'll work; if it doesn't I'll just remove it so we can get this version out.

That + re-render should be all you need for getting python 3.11 builds working

mikemhenry commented 1 year ago

@mattwthompson sorry I should have asked you before I made a commit, all I did was fix the sha256 hash

mattwthompson commented 1 year ago

No problem, that just saves us the hours (?) of CI cycles to fix that

mattwthompson commented 1 year ago

I'm seeing an error bubble up from pytraj:


2023-06-19T19:07:21.9950278Z pytraj/core/c_dict.cpp: In function 'void __Pyx_AddTraceback(const char*, int, int, const char*)':
2023-06-19T19:07:21.9951128Z pytraj/core/c_dict.cpp:460:62: error: invalid use of incomplete type 'PyFrameObject' {aka 'struct _frame'}
2023-06-19T19:07:21.9951541Z   460 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)
2023-06-19T19:07:21.9952044Z       |                                                              ^~
2023-06-19T19:07:21.9952388Z pytraj/core/c_dict.cpp:5597:5: note: in expansion of macro '__Pyx_PyFrame_SetLineNumber'
2023-06-19T19:07:21.9952864Z  5597 |     __Pyx_PyFrame_SetLineNumber(py_frame, py_line);
2023-06-19T19:07:21.9953083Z       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
2023-06-19T19:07:21.9953500Z In file included from /home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_/include/python3.11/Python.h:42,
2023-06-19T19:07:21.9953908Z                  from pytraj/core/c_dict.cpp:20:
2023-06-19T19:07:21.9954693Z /home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_/include/python3.11/pytypedefs.h:22:16: note: forward declaration of 'PyFrameObject' {aka 'struct _frame'}
2023-06-19T19:07:21.9955199Z    22 | typedef struct _frame PyFrameObject;
2023-06-19T19:07:21.9955644Z       |                ^~~~~~
2023-06-19T19:07:22.3680288Z error: command '/home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_build_env/bin/x86_64-conda-linux-gnu-cc' failed with exit code 1
2023-06-19T19:07:22.3979381Z make[2]: *** [AmberTools/src/pytraj/CMakeFiles/pytraj.dir/build.make:533: AmberTools/src/pytraj/CMakeFiles/python-build/pytraj-build.stamp] Error 1

mattwthompson commented 1 year ago

@conda-forge-admin, please rerender

mikemhenry commented 1 year ago

That error is the same as https://github.com/conda-forge/ambertools-feedstock/pull/113#issuecomment-1596037085 (I think) and they need to re-run cython to generate python 3.11 stuff that is ABI

dacase commented 1 year ago

OK: I did (try to) re-cythonize everything, using the latest cython and python 3.11. But I must have failed in some respect. I'll see if I can get Hai Nguyen (author of pytraj) to actually do this himself -- he keeps telling me what I ought to do, but that is no substitute.

I'm not sure what Hai's availability is like these days. He claims he did all this, and tested the result, but I've not yet been able to get the final result from him, and I don't see updates yet upstream (at github.com/Amber-MD/pytraj.)

Apologies for causing others to have to look at this. I personally don't understand what the interaction is between cpp files and python versions; that is, why should a .cpp file work OK for me but fail to compile with conda-forge compilers?

mikemhenry commented 1 year ago

No worries, there are a lot of moving parts here, I would start by having him check if this error:

2023-06-19T19:07:21.9950278Z pytraj/core/c_dict.cpp: In function 'void __Pyx_AddTraceback(const char*, int, int, const char*)':
2023-06-19T19:07:21.9951128Z pytraj/core/c_dict.cpp:460:62: error: invalid use of incomplete type 'PyFrameObject' {aka 'struct _frame'}
2023-06-19T19:07:21.9951541Z   460 |   #define __Pyx_PyFrame_SetLineNumber(frame, lineno)  (frame)->f_lineno = (lineno)
2023-06-19T19:07:21.9952044Z       |                                                              ^~
2023-06-19T19:07:21.9952388Z pytraj/core/c_dict.cpp:5597:5: note: in expansion of macro '__Pyx_PyFrame_SetLineNumber'
2023-06-19T19:07:21.9952864Z  5597 |     __Pyx_PyFrame_SetLineNumber(py_frame, py_line);
2023-06-19T19:07:21.9953083Z       |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
2023-06-19T19:07:21.9953500Z In file included from /home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_/include/python3.11/Python.h:42,
2023-06-19T19:07:21.9953908Z                  from pytraj/core/c_dict.cpp:20:
2023-06-19T19:07:21.9954693Z /home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_h_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_/include/python3.11/pytypedefs.h:22:16: note: forward declaration of 'PyFrameObject' {aka 'struct _frame'}
2023-06-19T19:07:21.9955199Z    22 | typedef struct _frame PyFrameObject;
2023-06-19T19:07:21.9955644Z       |                ^~~~~~
2023-06-19T19:07:22.3680288Z error: command '/home/conda/feedstock_root/build_artifacts/ambertools_1687199516276/_build_env/bin/x86_64-conda-linux-gnu-cc' failed with exit code 1
2023-06-19T19:07:22.3979381Z make[2]: *** [AmberTools/src/pytraj/CMakeFiles/pytraj.dir/build.make:533: AmberTools/src/pytraj/CMakeFiles/python-build/pytraj-build.stamp] Error 1

Is a new one or not (or indicates an issue with the cython-izing).

I personally don't understand what the interaction is between cpp files and python versions; that is, why should a .cpp file work OK for me but fail to compile with conda-forge compilers

This is a good question... It looks like conda-forge is using cython 0.29.35 does that match the version you are using?

hainm commented 1 year ago

He claims he did all this, and tested the result, but I've not yet been able to get the final result from him, and I don't see updates yet upstream (at github.com/Amber-MD/pytraj.)

Yeah, I did spend my whole afternoon of my vacation to test this. :D (But I only tested the step of forcing to re-cythonize the files. Basically tried to reproduce the success of the user in here: https://github.com/Amber-MD/pytraj/issues/1632#issuecomment-1586964715).

and I don't see updates yet upstream (at github.com/Amber-MD/pytraj.)

Because pytraj repo does not have the .cpp files. Those files only exist when you do the mkrelease_at.

dacase commented 1 year ago

I am using cython 0.29.35. However, the core/c_dict.cpp file did not get changed, and that is the file causing the error reported above. I can try to run things again, paying attention to that particular file.

hainm commented 1 year ago

I am using cython 0.29.35. However, the core/c_dict.cpp file did not get changed, and that is the file causing the error reported above. I can try to run things again, paying attention to that particular file.

I've been looking at the the tar file too. Those are unchanged files

/* Generated by Cython 0.29.25 */
./pytraj/core/c_dict.cpp
./pytraj/analysis/c_analysis/c_analysis.cpp

I suggest to delete ALL cpp files in pytraj before running mkrelease_at. I tested above assumption by doing below

download AmberTools23.3.tar.bz2 and untar it
compile and reproduce the reported failure.
delete all pytraj's cpp files.
run AmberTools/src/pytraj/devtools/mkrelease --amber (since there is no mkrelease_at) (I needed to remove all lines of code using git in that file).
make pytraj again and ok

mattwthompson commented 1 year ago

This version seems to be building fine as-is - could we review and merge this and tackle the Python 3.11 issues in a separate build?

dacase commented 1 year ago

THe only changes here were intended to deal with the python 3.11 issue. So there is nothing to merge if we plan to "tackle the python 3.11 issues in a separate build".

Hai has sent me some additional files he has that may address the error reported above. I'll try to get these merged in tomorrow.

mattwthompson commented 1 year ago

@conda-forge-admin, please rerender

mattwthompson commented 1 year ago

The build failed. I'm not sure what to make of the logs. There's a sea of warnings and then gcc throws an error. I'm probably missing something.

Here is one of the logs, which is not likely a permanent link: https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_apis/build/builds/731478/logs/25

hainm commented 1 year ago

@dacase I think there is still file(s) using cython 0.29.25 (instead of 0.29.35).

dacase commented 1 year ago

Hai is correct: there are still files that are cythonized with the wrong version of cython. Sorry for causing more wasted time. Don't waste any more time with this PR for now.

mikemhenry commented 1 year ago

@dacase Really no worries! Like I said there are a lot of moving parts. I've got access to the amber gitlab instance so if you want to reach out and email me, I can help!

Developing software and packaging software are two different concerns, so I am happy to help.

mikemhenry commented 1 year ago

Okay new plan (from David):

use the original AmberTools23_rc6.tar.bz2 as the input tarball
keep the update_amber part of build.sh
before building, do this in the top-level directory:

patch -p 0 < update.3 (update.3 is attached)
continue with python 3.11

I will first try adding the patch how we normally patch things

hainm commented 1 year ago

this is a good sign. :D

mikemhenry commented 1 year ago

:tada:

So what "version" do we call this? is 23.3 correct?

mikemhenry commented 1 year ago

I needed to reset the build back to zero, but it passes :sunglasses:

dacase commented 1 year ago

23.3 is the correct version number. I'll post update.3 to the Amber web site, so users getting 23.3 either from conda-forge or by building from source will be in sync.

Thanks again for working on this.

mikemhenry commented 1 year ago

@dacase Awesome! Do you want me to wait for the update to land on the website or is this good to merge in/release now? I think the only difference would be the patch for the cython stuff.

dacase commented 1 year ago

The update is up on the Amber web site -- you can go ahead and create/publish the 23.3 conda-forge package.

mattwthompson commented 1 year ago

If I'm following things correctly, this is good to merge? I'd give it the green sticker but I can't since I'm (hilariously) the original author

mattwthompson commented 1 year ago

@conda-forge-admin, please rerender

github-actions[bot] commented 1 year ago

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

I tried to rerender for you, but it looks like there was nothing to do.

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

dacase commented 1 year ago

We had to re-run cython on about 20 files, and the resulting changes create a fairly big patch file. But this is indeed 23.3, i.e. the 10 MB of changes are with respect to 23.0. (See above comments for why we don't need conda-forge packages for 23.1 and 23.2.)

dacase commented 1 year ago

Another note about this PR. It's packaged here as recipe/patches/fix_cython.patch but that is not really needed now. Since update.3 has been posted on the Amber web site, all these changes would be pulled in by the update_amber step, and fix_cython.patch is no longer needed.

This all happened because the upstream (Amber) CI is not testing python 3.11, so we were fighting a chicken-vs-egg battle to have conda-forge do the python 3.11 testing for us. It should be possible (and indeed desirable) to remove fix_cython.patch and make sure that everything is still OK.

mattwthompson commented 1 year ago

Does update_amber automagically pull files from the website/an Amber server? If not, we need to include the files themselves in the feedstock

Is there a difference between grabbing https://ambermd.org/downloads/AmberTools23_rc6.tar.bz2 and running updates vs. https://ambermd.org/downloads/AmberTools23.3.tar.bz2 or similar and running updates?

Are versions ultimately defined by the number of updates that are applied, and not only the contents of the tarball? That's how it currently works, so I assume that's the intended behavior.

mikemhenry commented 1 year ago

Sorry @jaimergp I didn't explain how the patch was temporary until the update landed on the website. Now that it is live I'll remove the patch and we can have the update script take care of the patching.

jaimergp commented 1 year ago

All good, just make sure to merge with squashing so the big patch doesn't get in the main branch history.

And if full archives are the way to get updates now (preferred imo), please remove this block from build.sh.

mikemhenry commented 1 year ago

In this issue https://github.com/conda-forge/ambertools-feedstock/issues/117 @dacase makes the case for why using a full archive doesn't fit their workflow.

dacase commented 1 year ago

Does update_amber automagically pull files from the website/an Amber server?

Yes.

Is there a difference between grabbing https://ambermd.org/downloads/AmberTools23_rc6.tar.bz2 and running updates vs. https://ambermd.org/downloads/AmberTools23.3.tar.bz2 or similar and running updates?

Should be no difference. I've actually removed the 23.3.tar.bz2 from the downloads directory on the website. I think we should continue to process updates via update_amber rather than to go to creating a new 600 MB tarfile every time we have an update.

Are versions ultimately defined by the number of updates that are applied

Yes. Mainly, this is the "dot" numberbering scheme we have been using for upstream (source) builds for quite a while.

conda-forge / ambertools-feedstock

Release 23.3 #118