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

Set run_exports max_pin to x.x #11

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

jdblischak
Copy link
Member

@jdblischak jdblischak commented Jan 15, 2024

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

When we originally added the Azure SDK for C++ libraries to conda-forge, we decided to set max_pin="x" (conda-forge/staged-recipes#23297 (comment)). This was for two primary motivations:

  1. Upstream said that minor versions were backwards compatible
  2. Strict run_exports make it much more difficult for the conda solver to find a valid environment. Thus we want to be as permissive as possible

However, we recently had our first consequence of this loose pinning. azure-core-cpp 1.11.0 is breaking tiledb conda binaries that were built against azure-core-cpp 1.10.3 (conda-forge/tiledb-feedstock#228).

This is a draft. Please do not merge. We need time for:

  • Allow for discussion and obtain approvals from multiple maintainers. The alternative to this PR is to keep the loose pin and hope that these breaking changes in minor releases are rare. Making max_pin more strict also has consequences on recipe maintenance. Recipes that build against azure-core-cpp will have to be rebuilt more regularly if they want to use the latest azure-core-cpp, and if enough recipes start depending on it, we'll need to add it to conda-forge-pinning
  • I need to submit a repodata patch for the currently uploaded tiledb to pin azure-core-cpp <1.11. If this was merged before this, that would once again break tiledb

And thinking longer-term: should we also update the max_pin for the other azure-*-cpp recipes? Or should we wait until they actually break backwards compatibility first? My current vote would be to wait on the other recipes, due to the extra solver constraints and maintenance of downstream recipes if we bump `max_bin="x.x"

@jdblischak jdblischak self-assigned this Jan 15, 2024
@conda-forge-webservices
Copy link
Contributor

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.

@jdblischak jdblischak changed the title Set run_exports max_pin to x.x [Do Not Merge] Set run_exports max_pin to x.x Jan 15, 2024
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you, @jdblischak

This LGTM modulo a 🟢 CI.

And thinking longer-term: should we also update the max_pin for the other azure-*-cpp recipes? Or should we wait until they actually break backwards compatibility first? My current vote would be to wait on the other recipes, due to the extra solver constraints and maintenance of downstream recipes if we bump `max_bin="x.x"

I think it would be safer to pin everything to max_pin="x.x" for safety and consistency.

@jdblischak
Copy link
Member Author

@Shelnutt2 @teo-tsirpanis Could you please weigh in on the following questions?

  • Short-term: should we pin the azure-core-cpp run exports to max_pin="x.x"? (ie merge this PR)
  • Longer-term: should we pin all the recipes for the Azure SDK for C++ libs to max_pin="x.x"?

The choice is a tradeoff. Using max_pin="x" means less maintenance rebuilding other recipes against each new azure-core-cpp release, but the downside is situations like the current one where 1.11.0 broke tiledb. Using max_pin="x.x" prevents problems like the current one, but then any conda package that wanted to use the latest version would have to be rebuilt against it. And depending on the frequency of new releases, we may eventually have to pin these in conda-forge-pinning in order to take advantage of the conda-forge automated PRs

@Shelnutt2
Copy link

Shelnutt2 commented Jan 17, 2024

@Shelnutt2 @teo-tsirpanis Could you please weigh in on the following questions?

* Short-term: should we pin the azure-core-cpp run exports to `max_pin="x.x"`? (ie merge this PR)

* Longer-term: should we pin all the recipes for the Azure SDK for C++ libs to `max_pin="x.x"`?

The choice is a tradeoff. Using max_pin="x" means less maintenance rebuilding other recipes against each new azure-core-cpp release, but the downside is situations like the current one where 1.11.0 broke tiledb. Using max_pin="x.x" prevents problems like the current one, but then any conda package that wanted to use the latest version would have to be rebuilt against it. And depending on the frequency of new releases, we may eventually have to pin these in conda-forge-pinning in order to take advantage of the conda-forge automated PRs

I'm of the opinion that the user experience is more important that the development experience. The breakage caused by this azire-core-cpp release was annoying and downstream users didn't initially understand the cause. I think using max_pin="x.x" is the best option. Agreed that if the updates are too frequent and the builds are too frequent we can look at multiple adjustments including conda-forge-pinning or changing the behavior of the pins again.

@jdblischak
Copy link
Member Author

Thanks all for the discussion. Now we're just waiting on conda-forge/conda-forge-repodata-patches-feedstock#639

Copy link
Member

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

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

I think I was the one who suggested pinning to x because that's what the Azure SDK promised. It's sad that 1.11 had a binary breaking change, which I could not identify after a quick look at the diffs between the versions. I'm not against pinning to x.x if the other maintainers think so. I'm also not against restoring the pin in the future if sufficient assurances are made by the Azure SDK team.

@jjerphan
Copy link
Member

jjerphan commented Jan 22, 2024

Thank you, @teo-tsirpanis.

I guess we miss your approval to merge this PR. Alternatively, I guess you can merge this PR.

@jdblischak: can we set this PR as ready for review, or is there something left to be done apart from conda-forge/conda-forge-repodata-patches-feedstock#639?

@jdblischak
Copy link
Member Author

can we set this PR as ready for review, or is there something left to be done apart from conda-forge/conda-forge-repodata-patches-feedstock#639?

@jjerphan that PR is the final step. I don't want this one merged into that one has been.

I think we've reached a consensus among the maintainers that this PR will be merged when ready:

  • Strong support for merging: Julien, Seth
  • Hesitant but still support to prevent similar problems in the future: John, Theodore

After this one is merged, I'll open a PR in a downstream feedstock, and we can continue the discussion there on whether or not to apply max_pin="x.x" to all of them.

@jdblischak jdblischak changed the title [Do Not Merge] Set run_exports max_pin to x.x Set run_exports max_pin to x.x Jan 30, 2024
@jdblischak jdblischak marked this pull request as ready for review January 30, 2024 20:45
@jdblischak
Copy link
Member Author

My repodata patch was merged today, so existing tiledb binaries built against azure-core-cpp 1.10 will now be installed with 1.10 at runtime conda-forge/conda-forge-repodata-patches-feedstock@c7bc734

@teo-tsirpanis
Copy link
Member

Azure/azure-sdk-for-cpp#5322 (comment)

Does this mean we should pin all Azure SDK packages to x.x.x?

@jdblischak
Copy link
Member Author

@teo-tsirpanis thanks for opening that detailed Issue upstream

Does this mean we should pin all Azure SDK packages to x.x.x?

My interpretation is yes. They clearly aren't testing binary compatibility across releases, so I don't think we should assume it

I looked at CMake's write_basic_package_version_file. Unfortunately it doesn't distinguish between source and binary compatibility

Any other thoughts?

@teo-tsirpanis
Copy link
Member

Just saw the AWS SDK feedstocks pin all packages (even the C libraries) to x.x.x. We probably should do the same in the Azure SDK.

@jdblischak
Copy link
Member Author

Does this mean we should pin all Azure SDK packages to x.x.x?

Finalizing this thread:

@jjerphan
Copy link
Member

Thank you for taking care of this, @jdblischak.

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.

4 participants