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

tiledbsoma 1.11.2 #157

Merged
merged 5 commits into from
May 23, 2024
Merged

tiledbsoma 1.11.2 #157

merged 5 commits into from
May 23, 2024

Conversation

johnkerl
Copy link
Contributor

Following our established procedure

@jdblischak
Copy link
Collaborator

Surprisingly the osx-* builds passed but the linux-64 Python builds failed to obtain the version in this setup where RELEASE-VERSION is artificially created

echo "$PKG_VERSION" >> RELEASE-VERSION

Here's the traceback:

  File "/home/conda/feedstock_root/build_artifacts/tiledbsoma_1716331838735/work/apis/python/version.py", line 275, in get_version
    version = get_git_version()
              ^^^^^^^^^^^^^^^^^
  File "/home/conda/feedstock_root/build_artifacts/tiledbsoma_1716331838735/work/apis/python/version.py", line 223, in get_git_version
    latest_tag = get_latest_remote_tag(remote)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/conda/feedstock_root/build_artifacts/tiledbsoma_1716331838735/work/apis/python/version.py", line 133, in get_latest_remote_tag
    raise RuntimeError(f"No tags found in remote {remote}")
RuntimeError: No tags found in remote origin
error: subprocess-exited-with-error

This error is surprising given the recent updates to build from an sdist that led to closing single-cell-data/TileDB-SOMA#2588

@johnkerl
Copy link
Contributor Author

cc @ryan-williams

@johnkerl
Copy link
Contributor Author

@jdblischak
Copy link
Collaborator

jdblischak commented May 22, 2024

I was able to build it locally from the 1.11.2 GitHub release tarball using Python 3.9:

mamba create --yes -n test-soma python=3.9 pip
mamba activate test-soma

wget https://github.com/single-cell-data/TileDB-SOMA/archive/refs/tags/1.11.2.tar.gz
tar xzf 1.11.2.tar.gz
cd TileDB-SOMA-1.11.2/

git status
## fatal: not a git repository (or any of the parent directories): .git

cd apis/python/
echo "1.11.2" >> RELEASE-VERSION
python -m pip install . -vv

This of course isn't identical to the conda build setup (since I didn't pre-install libtiledb or libtiledbsoma), but I don't think that should affect the logic of version.py.

@jdblischak
Copy link
Collaborator

Also, @johnkerl, how are you preventing the Python 3.12 variants from being added during the rerendering like they are in the nightly-build branch, eg 878194b?

@johnkerl
Copy link
Contributor Author

Also, @johnkerl, how are you preventing the Python 3.12 variants from being added during the rerendering like they are in the nightly-build branch, eg 878194b?

@jdblischak I'm not aware that I'm doing any preventing -- am I doing something wrong that I'm not aware of? Am I doing something right that I'm not aware of?

@jdblischak
Copy link
Collaborator

am I doing something wrong that I'm not aware of?

My only guess was that maybe you edited conda_build_config.yaml like I had yesterday (#153 (comment)) but then didn't add and commit the file. A quick git status should confirm or deny this possibility

Am I doing something right that I'm not aware of?

If my above guess is wrong, then don't change anything! :-) You might have a slightly older conda-smithy installed locally, but we don't want those Python 3.12 variants anyways.

@@ -19,11 +19,13 @@ package:
source:
url: https://github.com/single-cell-data/TileDB-SOMA/archive/{{ version }}.tar.gz
sha256: {{ sha256 }}
patches: # [osx and not arm64]
- 0001-version-py-workaround-1-11-2.patch

Copy link
Collaborator

Choose a reason for hiding this comment

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

A few notes:

  1. I think we should apply the patch to all the builds. We know code will be in the next release, so we may as well test it now
  2. I don't think the Jinja2 preprocessing selectors are that smart. I think you have to apply it to each line you want to remove
  3. The version.py issues are with the linux-64 builds, so if we are going to selectively apply the patch, I think we would use # [linux]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should apply the patch to all the builds. We know code will be in the next release, so we may as well test it now

Sorry, I lack your expersite -- how am I not? What am I doing wrong?

I don't think the Jinja2 preprocessing selectors are that smart

Here too -- where is Jinja involved here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would use # [linux]

This one I think I understand -- mindless copypasta from my recent work on r-tiledb-feedstock -- I'll update ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c51fec9
🤞

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here too -- where is Jinja involved here?

The preprocessing selectors like [linux] and [osx and not arm64] are processed by the Python package jinja2. If the variable is true, the line is kept. If the variable is false, the line is removed from the recipe.

My first suggestion was to remove all of the selectors from the patch, so that the patch would be applied to all of the builds. You implemented my other suggestion, which was to use # [linux]. This is fine since it applies the patch only to the linux builds, which is what we need for green CI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK thanks @jdblischak ... as always I creep slowly in the direction of understanding when it comes to Conda ... 🤞

@ryan-williams
Copy link
Member

ryan-williams commented May 23, 2024

Previous build seems to have failed with:

RuntimeError: SHA256 mismatch: 'e3933c7d7b3d8df59a40ed7833fb20774917f531a915debc76abb569e958a421' != 'e3933c7d7b3d8df59a40ed7833fb20774917f531a915debc76abb569e958a421 - 0001-version-py-workaround-1-11-2.patch'

link

The SHA parts match, but the right side contains an extra - 0001-version-py-workaround-1-11-2.patch 🤔

@jdblischak
Copy link
Collaborator

The SHA parts match, but the right side contains an extra - 0001-version-py-workaround-1-11-2.patch 🤔

@ryan-williams See my comment above, #157 (comment), about jinja2 not being that smart. This confirms my understanding. It has no understanding of the YAML sections, but is simply applied line by line. So what happened was that the field patches: was removed from the linux-64 builds (since [osx and not arm64] evaluates to false on a linux-64 build), so the following line with the path to the patch file looked like it belonged to the field sha256, and hence the failure.

@johnkerl johnkerl merged commit 3a0d9dc into main May 23, 2024
5 checks passed
@johnkerl johnkerl deleted the kerl/tiledbsoma-1.11.2 branch May 23, 2024 21:29
@johnkerl
Copy link
Contributor Author

johnkerl commented May 23, 2024

about jinja2 not being that smart

For my future reference: this means operationally: if some yaml file has

foo: bar # [baz]
    quux # [xyzzy]

this is not good -- such lines need to be like

foo: bar #[baz]
    quux #[baz]

where the baz thing need to evaluate to true or false for each #-tagged line that the author thinks of as belonging together

@johnkerl
Copy link
Contributor Author

@johnkerl
Copy link
Contributor Author

... success!! :)

Now I'll monitor

https://anaconda.org/tiledb/libtiledbsoma/labels
https://anaconda.org/tiledb/tiledbsoma-py/labels
https://anaconda.org/tiledb/r-tiledbsoma/labels

to verify in the next hour or two that 1.11.2 artifacts arrive. This will prove we've gotten the artifact-creation issue from this release, confined to this release.

@johnkerl
Copy link
Contributor Author

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

Successfully merging this pull request may close these issues.

3 participants