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

fix: swaps from github API to expanding the assets #907

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

holtjma
Copy link
Contributor

@holtjma holtjma commented Jul 28, 2023

This is basically a patch to fix a new issue (described here: bioconda/bioconda-recipes#42176) that was introduced by myself here: #896.

The primary cause is that the GitHub API is rate limited (which I was not aware of), so autobump basically blows through the limits quickly and then subsequent calls automatically fail. Of course, this means that the tools we were hoping to see get autobumped are not.

This patch basically removes the original patch (which uses a single GitHub API call) and replaces it with an approach that basically "expands" the release webpage to find the assets. See #895 for more details on how the URLs are masked and the expansion pattern.

Couple other notes:

  • I added an IncludeFragmentParser that is basically identical to the HrefParser except it is looking for tags with this pattern: <include-fragment src="http://url/of/interest" ...>
  • The general expansion logic is:
    • Pull the full release page
    • Search for <include-fragment ...> tags
    • Iteratively pull these looking for the main link URL of interest
    • Break the above iteration early if the current version is identified
  • This should no longer use the GitHub API, I'm unaware of other rate limiters for these URLs (🤞that none are hidden...)

Pinging @dpryan79 and @johanneskoester as you both reviewed the last related PR. Sorry that the previous PR injected a rate limit issue!

EDIT: I forgot, I have tested these on both hiphase and pbfusion recipes, the diff of the results are here and match our current expectations:

(bioconda)$ git diff
diff --git a/recipes/hiphase/meta.yaml b/recipes/hiphase/meta.yaml
index 38b939bf9f..b5d2503597 100644
--- a/recipes/hiphase/meta.yaml
+++ b/recipes/hiphase/meta.yaml
@@ -1,6 +1,6 @@
 {% set name = "hiphase" %}
-{% set version = "0.10.0" %}
-{% set hiphase_sha256 = "e22d3530a395bd218bd4634537f5b3c31a82cd8155849291c69d81c48f0c58fe" %}
+{% set version = "0.10.2" %}
+{% set hiphase_sha256 = "2a20fc437ab5c0f2437246e8a36e7590c68f5ae5caa0cecb8e85a371f5357c6c" %}
 
 package:
   name: {{ name }}
diff --git a/recipes/pbfusion/meta.yaml b/recipes/pbfusion/meta.yaml
index 781ada3413..ca69e87d0f 100644
--- a/recipes/pbfusion/meta.yaml
+++ b/recipes/pbfusion/meta.yaml
@@ -1,6 +1,6 @@
 {% set name = "pbfusion" %}
-{% set version = "0.2.2" %}
-{% set pbfusion_sha256 = "3390adbeb8550265fcf4fad89dad80740cc368e96d10f3b23bb19ef001ffd081" %}
+{% set version = "0.3.0" %}
+{% set pbfusion_sha256 = "ef9767af7e82bd1e664711e7521d7a8370a41f101cccf1db4a58895d9f88934c" %}
 
 package:
   name: {{ name }}

@holtjma holtjma changed the title swaps from github API to expanding the assets fix: swaps from github API to expanding the assets Jul 28, 2023
@holtjma
Copy link
Contributor Author

holtjma commented Jul 29, 2023

That failure seems odd, almost looks like it froze for some reason when I checked timestamps. I would restart it, but I don't think I can.

@holtjma
Copy link
Contributor Author

holtjma commented Jul 31, 2023

Looks like checks passed now, just waiting on feedback from maintainers on the change

Copy link
Contributor

@johanneskoester johanneskoester left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks a lot! Of course I would prefer the API as we never know what the do with the page layout. Maybe we can keep the API code as well, and just have both options? This way, we could later decide to use a rate limiter or maybe convince github of some higher limits without having to implement everything again.

@johanneskoester johanneskoester self-assigned this Aug 9, 2023
@jmarshall
Copy link
Member

See my comment on bioconda/bioconda-recipes#42176: if we store etags from the API calls, it’d be trivial to avoid the rate limits.

@holtjma
Copy link
Contributor Author

holtjma commented Aug 9, 2023

Maybe we can keep the API code as well, and just have both options? This way, we could later decide to use a rate limiter or maybe convince github of some higher limits without having to implement everything again.

@johanneskoester I moved the API version and the expanded asset version into subroutines that are invoked. John's point about the etags / last modified is probably the best way to handle the API in the future, but I'm not sure what long-term memory access autobump would have to retain that info. Those modifications could probably exist in a future PR though.

Tests appear to be passing again, and I double checked that results were the same with hiphase and pbfusion recipes.

@johanneskoester johanneskoester merged commit 2099a40 into bioconda:master Aug 16, 2023
@holtjma holtjma deleted the fix_alternate_lookup_limit branch August 24, 2023 20:00
johanneskoester pushed a commit that referenced this pull request Aug 25, 2023
…ts (#912)

With #907, we resolved an issue where GitHub release files were not
being detected via autobump. Recent logs (e.g.
https://circleci.com/api/v1.1/project/github/bioconda/bioconda-utils/44398/output/110/0?file=true&allocation-id=64e7a915cc25c03a3d0cf725-0-build%2FABCDEFGH)
show this has changed and it is correctly detecting the version update.
However, we are generating new errors now that are specifically caused
by the PR submission (this was not previously tested, an oversight on my
part):

```
19:09:44 �[32mBIOCONDA INFO�[0m Recipe hiphase: updating from remote bump/hiphase�[0m
19:09:46 �[31mBIOCONDA ERROR�[0m While processing hiphase
Traceback (most recent call last):
  File "/opt/mambaforge/envs/bioconda/lib/python3.8/site-packages/bioconda_utils/aiopipe.py", line 199, in process
    await filt.apply(item)
  File "/opt/mambaforge/envs/bioconda/lib/python3.8/site-packages/bioconda_utils/autobump.py", line 1142, in apply
    author = self.get_github_author(recipe)
  File "/opt/mambaforge/envs/bioconda/lib/python3.8/site-packages/bioconda_utils/autobump.py", line 1135, in get_github_author
    return ver['vals']['account']
KeyError: 'vals'�[0m
```

This is caused by a lack of passing the `vals` fields through from the
previous PR. Error was replicated locally via:
```
bioconda-utils autobump recipes/ config.yml --package hiphase --create-pr --dry-run
```

With the very minor changes here, `vals` is passed through and we now
get a full run without errors via dry-run:
```
13:35:00 BIOCONDA INFO Updating checksum for hiphase 0.10.2                                                                                                                                            
13:35:00 BIOCONDA INFO Checking out branch bump/hiphase                                                                                                                                                
13:35:06 BIOCONDA INFO Would create PR 'Update hiphase to 0.10.2'                                                                                                                                      
13:35:06 BIOCONDA INFO  title: Update hiphase to 0.10.2                                                                                                                                                
13:35:06 BIOCONDA INFO  body:                                                                                                                                                                          
<!--
creator: autobump
type: bump_version
recipe: hiphase
orig_version: 0.10.0
orig_build_number: 0
new_version: 0.10.2
new_build_bumber: 0
-->

Update [`hiphase`](https://bioconda.github.io/recipes/hiphase/README.html): **0.10.0** &rarr; **0.10.2**

[![install with bioconda](https://img.shields.io/badge/install%20with-bioconda-brightgreen.svg?style=flat)](http://bioconda.github.io/recipes/hiphase/README.html) [![Conda](https://img.shields.io/conda/dn/bioconda/hiphase.svg)](https://anaconda.org/bioconda/hiphase/files)

Info | Link or Description
-----|--------------------
Recipe | [`recipes/hiphase`](https://github.com//bioconda/bioconda-recipes/tree/bump/hiphase/recipes/hiphase) (click to view/edit other files)
Summary | Small and structural variant phasing tool for PacBio HiFi reads
Home | [https://github.com/PacificBiosciences/HiPhase](https://github.com/PacificBiosciences/HiPhase)
Releases |[]()
Recipe Maintainer(s) | @holtjma, @ctsa
Author | `@PacificBiosciences`
***


This pull request was automatically generated (see [docs](https://bioconda.github.io/contributor/updating.html)).

13:35:06 BIOCONDA INFO Would modify PR -1                                                                                                                                                              
13:35:06 BIOCONDA INFO New labels: ['autobump', 'new version']                                                                                                                                         
13:35:06 BIOCONDA INFO Created PR -1: Update hiphase to 0.10.2                                                                                                                                         
100%|████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████████| 1/1 [00:11<00:00, 11.63s/it]
13:35:06 BIOCONDA WARNING Finished update
13:35:06 BIOCONDA INFO Unrecognized URL stats:
13:35:06 BIOCONDA INFO 
13:35:06 BIOCONDA INFO Recipe status statistics:
13:35:06 BIOCONDA INFO Updated: 1
13:35:06 BIOCONDA INFO SUM: 1
13:35:06 BIOCONDA WARNING Switching back to master
```

Hopefully, this is the last PR related to this issue!
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