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

Update nextstrain-cli's dependency on s3fs #39711

Merged

Conversation

tsibley
Copy link
Contributor

@tsibley tsibley commented Mar 2, 2023

Adds the same minimum version bound used by our Python package on PyPI. This lower bound is not strictly necessary for functionality, but it ensures that this Conda package ends up with an s3fs version that uses aiobotocore instead of much older s3fs versions that don't. The increased parity between the Python and Conda packages is useful to avoid compatibility bugs only observed in one packaging or the other.

Related-to: nextstrain/cli#261


Please read the guidelines for Bioconda recipes before opening a pull request (PR).

  • If this PR adds or updates a recipe, use "Add" or "Update" appropriately as the first word in its title.
  • New recipes not directly relevant to the biological sciences need to be submitted to the conda-forge channel instead of Bioconda.
  • PRs require reviews prior to being merged. Once your PR is passing tests and ready to be merged, please issue the @BiocondaBot please add label command.
  • Please post questions on Gitter or ping @bioconda/core in a comment.
Please use the following BiocondaBot commands:

Everyone has access to the following BiocondaBot commands, which can be given in a comment:

@BiocondaBot please update Merge the master branch into a PR.
@BiocondaBot please add label Add the please review & merge label.
@BiocondaBot please fetch artifacts Post links to CI-built packages/containers.
You can use this to test packages locally.

For members of the Bioconda project, the following command is also available:

@BiocondaBot please merge Upload built packages/containers and merge a PR.
Someone must approve a PR first!
This reduces CI build time by reusing built artifacts.

Also, the bot watches for comments from non-members that include @bioconda/<team> and will automatically re-post them to notify the addressed <team>.

@tsibley
Copy link
Contributor Author

tsibley commented Mar 2, 2023

Marking this PR as a draft until I see CI pass, as I'm not sure how the Conda solver will handle the complex resolution of s3fs, aiobotocore, botocore, and boto3 given this change.

@tsibley tsibley force-pushed the update-nextstrain-cli-s3fs-dep branch from c4929ba to 486b155 Compare March 2, 2023 22:15
@tsibley tsibley marked this pull request as ready for review March 2, 2023 23:37
@tsibley
Copy link
Contributor Author

tsibley commented Mar 2, 2023

@BiocondaBot please add label

@tsibley
Copy link
Contributor Author

tsibley commented Mar 2, 2023

The macOS checks failed to get scheduled in a timely fashion, so they were auto-cancelled. I can't re-try them as I'm not a member of Bioconda. Can someone retrigger them?

@tsibley
Copy link
Contributor Author

tsibley commented Mar 3, 2023

@BiocondaBot please add label

@BiocondaBot BiocondaBot added the please review & merge set to ask for merge label Mar 3, 2023
Adds the same minimum version bound used by our Python package on PyPI.
This lower bound is not strictly necessary for functionality, but it
ensures that this Conda package ends up with an s3fs version that uses
aiobotocore instead of much older s3fs versions that don't.  The
increased parity between the Python and Conda packages is useful to
avoid compatibility bugs only observed in one packaging or the other.

Related-to: <nextstrain/cli#261>
@tsibley tsibley force-pushed the update-nextstrain-cli-s3fs-dep branch from 486b155 to 7392d71 Compare March 3, 2023 19:12
@tsibley
Copy link
Contributor Author

tsibley commented Mar 3, 2023

Repushed to rebase on top of latest master and trigger new macOS CI runs that will hopefully get scheduled this time.

@daler
Copy link
Member

daler commented Mar 6, 2023

@tsibley I'm having trouble figuring out why the macOS CI run thinks there's nothing to do. Do you want to merge this in and get the linux package up (and possibly try a build number bump with macOS later) or continue debugging here?

@tsibley
Copy link
Contributor Author

tsibley commented Mar 6, 2023

@daler Hmm. Let's debug here if we can? I'd rather not have discordant packages between Linux and macOS.

I don't have any immediate ideas as to why the CI here thinks there's nothing to build for macOS, but Bioconda's CI is largely a black box to me currently. I compared the logs a bit between the Linux and macOS CI runs, and there are some notable differences (Linux restored from cache, macOS did not; macOS has an extensive "OSX setup" step that Linux does not), but none that jump out to me as explanatory here. Both runs seem to have gotten the correct copy of the repo: 63e6498, which is the auto-merge of this PR (7392d71) into master (f86881d).

Is there someone with more existing experience with Bioconda's CI we can flag this issue to?

@daler
Copy link
Member

daler commented Mar 7, 2023

Now that I look closer, there are some recent PRs where macOS packages did get built during testing, and some equally recent ones where it did not. It may be that you're the first to notice . . .

In any case hopefully the differences between the builds and the builds-not will be informative in debugging.

We all wrote different parts of the infrastructure at different times, so also pinging @dpryan79 @bgruening @epruesse @johanneskoester @rpetit3 for some more eyes on this.

For those of you running across this now, the problem is that in some -- but not all -- cases, bioconda-utils thinks there's nothing to do for macOS, even though the recipe does not have anything about skipping osx. This results in the macOS test passing (and so it's easy to miss this because we see the false positive and think everything's fine....)

@tsibley
Copy link
Contributor Author

tsibley commented Mar 8, 2023

@daler Oh! False alarm, maybe? This is a noarch package. Do macOS builds happen for noarch packages?

My initial comment about the macOS runs was because the actual jobs didn't get scheduled on a runner before some timeout, so they never happened and were shown as canceled in GitHub's UI. Once I repushed, they did get scheduled and run, showing as successful here. You then pointed out that they didn't actually build anything, which seemed concerning, but I realize now that maybe it's because this is a noarch: python package.

@tsibley
Copy link
Contributor Author

tsibley commented Mar 8, 2023

Indeed, that appears to be the case. This repo's CI calls bioconda-utils build:

bioconda-utils build recipes config.yml \
--git-range origin/"$SYSTEM_PULLREQUEST_TARGETBRANCH" HEAD

which calls this function chain:

bioconda_utils.cli.build
bioconda_utils.build.build_recipes
bioconda_utils.utils.get_package_paths
bioconda_utils.utils.check_recipe_skippable

which returns true to skip the recipe (and produce no package paths and thus nothing to build) when the recipe is noarch and not building for Linux:

https://github.com/bioconda/bioconda-utils/blob/bfafdf5dcee0a557705763399af7b0487fe4f883/bioconda_utils/utils.py#L1057-L1065

@tsibley
Copy link
Contributor Author

tsibley commented Mar 8, 2023

@BiocondaBot please merge

@daler
Copy link
Member

daler commented Mar 8, 2023

Oh, I was being dumb, yes, you are totally right! I missed that this was noarch.

(to all pinged: sorry for the noise)

Linux resources are more plentiful, so if architecture doesn't matter like for noarch, then only Linux builds run and no need to waste macOS runners. So we're good.

All that said, I think it'll be worth adding a log message saying as much.

@BiocondaBot
Copy link
Collaborator

I will attempt to upload artifacts and merge this PR. This may take some time, please have patience.

@BiocondaBot BiocondaBot merged commit 1a1a420 into bioconda:master Mar 8, 2023
@tsibley
Copy link
Contributor Author

tsibley commented Mar 8, 2023

@daler There is a log message:

logger.debug('FILTER: only building %s on '
             'linux because it defines noarch.',
             recipe)

but it's debug-level and that's seemingly not enabled for CI. It would have appeared right before this log line:

20:41:46 BIOCONDA INFO Nothing to be done for recipe recipes/nextstrain-cli

:-)

@tsibley
Copy link
Contributor Author

tsibley commented Mar 8, 2023

In any case, thanks for your attention here! 🙏

tsibley added a commit to nextstrain/cli that referenced this pull request Mar 15, 2023
This permits Micromamba to downgrade packages that are already installed
in order to upgrade nextstrain-base.  While nextstrain-base's
fully-locked dependencies from version to version will typically only
cause upgrades, there are times when downgrades may be required.¹  When
that's the case, Micromamba simply won't consider that nextstrain-base
version as available without --allow-downgrade.

Resolves <#264>.

¹ For example, the deps of Nextstrain CLI's package in Bioconda changed
  to better match its deps in PyPI², which meant versions of botocore,
  boto3, and some related packages had to be downgraded.

² <bioconda/bioconda-recipes#39711>
tsibley added a commit to nextstrain/cli that referenced this pull request Mar 21, 2023
This permits Micromamba to downgrade packages that are already installed
in order to upgrade nextstrain-base.  While nextstrain-base's
fully-locked dependencies from version to version will typically only
cause upgrades, there are times when downgrades may be required.¹  When
that's the case, Micromamba simply won't consider that nextstrain-base
version as available without --allow-downgrade.

Resolves <#264>.

¹ For example, the deps of Nextstrain CLI's package in Bioconda changed
  to better match its deps in PyPI², which meant versions of botocore,
  boto3, and some related packages had to be downgraded.

² <bioconda/bioconda-recipes#39711>
cokelaer pushed a commit to cokelaer/bioconda-recipes that referenced this pull request Apr 28, 2023
Merge PR bioconda#39711, commits were: 
 * Update nextstrain-cli's dependency on s3fs

Adds the same minimum version bound used by our Python package on PyPI.
This lower bound is not strictly necessary for functionality, but it
ensures that this Conda package ends up with an s3fs version that uses
aiobotocore instead of much older s3fs versions that don't.  The
increased parity between the Python and Conda packages is useful to
avoid compatibility bugs only observed in one packaging or the other.

Related-to: <nextstrain/cli#261>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please review & merge set to ask for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants