Download pipelines with authenticated GH API calls #3607
+79
−42
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |||||||||||||||||||||||||||||||||||||
| NF_INSPECT_MIN_NF_VERSION, | ||||||||||||||||||||||||||||||||||||||
| NFCORE_VER_LAST_WITHOUT_NF_INSPECT, | ||||||||||||||||||||||||||||||||||||||
| check_nextflow_version, | ||||||||||||||||||||||||||||||||||||||
| gh_api, | ||||||||||||||||||||||||||||||||||||||
| pretty_nf_version, | ||||||||||||||||||||||||||||||||||||||
| run_cmd, | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -47,7 +48,7 @@ class DownloadWorkflow: | |||||||||||||||||||||||||||||||||||||
| Can also download its Singularity container image if required. | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||
| pipeline (str | None): The name of an nf-core-compatible pipeline in the form org/repo | ||||||||||||||||||||||||||||||||||||||
| revision (tuple[str] | str | None): The workflow revision(s) to download, like `1.0` or `dev` . Defaults to None. | ||||||||||||||||||||||||||||||||||||||
| outdir (Path | None): Path to the local download directory. Defaults to None. | ||||||||||||||||||||||||||||||||||||||
| compress_type (str | None): Type of compression for the downloaded files. Defaults to None. | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,6 +62,7 @@ class DownloadWorkflow: | |||||||||||||||||||||||||||||||||||||
| container_cache_index (Path | None): An index for the remote container cache. Defaults to None. | ||||||||||||||||||||||||||||||||||||||
| parallel (int): The number of parallel downloads to use. Defaults to 4. | ||||||||||||||||||||||||||||||||||||||
| hide_progress (bool): Flag to hide the progress bar. Defaults to False. | ||||||||||||||||||||||||||||||||||||||
| authenticated (bool): If True, use the API of the SCM provider to download. Requires authentication e.g., via GITHUB_TOKEN. Defaults to False. | ||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def __init__( | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -79,6 +81,7 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
| container_cache_index: Path | None = None, | ||||||||||||||||||||||||||||||||||||||
| parallel: int = 4, | ||||||||||||||||||||||||||||||||||||||
| hide_progress: bool = False, | ||||||||||||||||||||||||||||||||||||||
| authenticated: bool = False, | ||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||
| # Verify that the flags provided make sense together | ||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -148,6 +151,8 @@ def __init__( | |||||||||||||||||||||||||||||||||||||
| self.container_cache_index = container_cache_index | ||||||||||||||||||||||||||||||||||||||
| # allows to specify a container library / registry or a respective mirror to download images from | ||||||||||||||||||||||||||||||||||||||
| self.parallel = parallel | ||||||||||||||||||||||||||||||||||||||
| self.hide_progress = hide_progress | ||||||||||||||||||||||||||||||||||||||
| self.authenticated = authenticated | ||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. We should still be able to read the authenticated status from
Suggested change
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| self.wf_revisions: list[dict[str, Any]] = [] | ||||||||||||||||||||||||||||||||||||||
| self.wf_branches: dict[str, Any] = {} | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -461,11 +466,19 @@ def get_revision_hash(self) -> None: | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| if not self.platform: | ||||||||||||||||||||||||||||||||||||||
| for revision, wf_sha in self.wf_sha.items(): | ||||||||||||||||||||||||||||||||||||||
| # Set the download URL - only applicable for classic downloads | ||||||||||||||||||||||||||||||||||||||
| if self.authenticated: | ||||||||||||||||||||||||||||||||||||||
| # For authenticated downloads, use the GitHub API | ||||||||||||||||||||||||||||||||||||||
| self.wf_download_url = { | ||||||||||||||||||||||||||||||||||||||
| **self.wf_download_url, | ||||||||||||||||||||||||||||||||||||||
| revision: f"https://api.github.com/repos/{self.pipeline}/zipball/{wf_sha}", | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||
| # For unauthenticated downloads, use the archive URL | ||||||||||||||||||||||||||||||||||||||
| self.wf_download_url = { | ||||||||||||||||||||||||||||||||||||||
| **self.wf_download_url, | ||||||||||||||||||||||||||||||||||||||
| revision: f"https://github.com/{self.pipeline}/archive/{wf_sha}.zip", | ||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||
JulianFlesch marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def prompt_config_inclusion(self) -> None: | ||||||||||||||||||||||||||||||||||||||
| """Prompt for inclusion of institutional configurations""" | ||||||||||||||||||||||||||||||||||||||
|
|
@@ -555,22 +568,31 @@ def prompt_compression_type(self) -> None: | |||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| def download_wf_files(self, revision: str, wf_sha: str, download_url: str) -> str: | ||||||||||||||||||||||||||||||||||||||
| """Downloads workflow files from GitHub to the :attr:`self.outdir`.""" | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| log.debug(f"Downloading {download_url}") | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Fetch content and determine top-level directory based on authentication method | ||||||||||||||||||||||||||||||||||||||
| if self.authenticated: | ||||||||||||||||||||||||||||||||||||||
| # GitHub API download: fetch via API and get topdir from zip contents | ||||||||||||||||||||||||||||||||||||||
| content = gh_api.get(download_url).content | ||||||||||||||||||||||||||||||||||||||
| with ZipFile(io.BytesIO(content)) as zipfile: | ||||||||||||||||||||||||||||||||||||||
| topdir = zipfile.namelist()[0] # API zipballs have a generated directory name | ||||||||||||||||||||||||||||||||||||||
| zipfile.extractall(self.outdir) | ||||||||||||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||||||||||||
| # Direct URL download: fetch and construct expected topdir name | ||||||||||||||||||||||||||||||||||||||
| content = requests.get(download_url).content | ||||||||||||||||||||||||||||||||||||||
| topdir = f"{self.pipeline}-{wf_sha if bool(wf_sha) else ''}".split("/")[-1] | ||||||||||||||||||||||||||||||||||||||
| with ZipFile(io.BytesIO(content)) as zipfile: | ||||||||||||||||||||||||||||||||||||||
| zipfile.extractall(self.outdir) | ||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+574
to
+586
There was a problem hiding this comment.
Suggested change
There was a problem hiding this comment. I am talking about replacing the old way of downloading with your Github API urls. This works also with unauthenticated requests (within a quota) and that way we can reduce complexity and remove the new parameter. What are your thoughts @MatthiasZepper ? |
||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Create a filesystem-safe version of the revision name for the directory | ||||||||||||||||||||||||||||||||||||||
| revision_dirname = re.sub("[^0-9a-zA-Z]+", "_", revision) | ||||||||||||||||||||||||||||||||||||||
| # Account for name collisions, if there is a branch / release named "configs" or container output dir | ||||||||||||||||||||||||||||||||||||||
| if revision_dirname in ["configs", self.get_container_output_dir()]: | ||||||||||||||||||||||||||||||||||||||
| revision_dirname = re.sub("[^0-9a-zA-Z]+", "_", self.pipeline + revision_dirname) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Rename the internal directory name to be more friendly | ||||||||||||||||||||||||||||||||||||||
| (self.outdir / topdir).rename(self.outdir / revision_dirname) | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
| # Make downloaded files executable | ||||||||||||||||||||||||||||||||||||||
| for dirpath, _, filelist in os.walk(self.outdir / revision_dirname): | ||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some reservations regarding the
--api-downloadoption from a user experience perspective. The current solution exposes implementation details that users shouldn't need to think about. Instead, we could focus on what the user actually wants to achieve - authenticated vs. anonymous downloads.Consider something like
--authenticatedand a help text like Enable authenticated download (with better rate limits, access to private repos, etc.) instead.Potentially, even
--auth-method <method>to future-proof it, if we want to support multiple authentication methods in the future. For example, the option to download pipelines not hosted on GitHub has also been a long-standing request and similar features could then be successively added without renaming / replacing too many CLI arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatthiasZepper while I agree with the switch to authenticated, I am not sure if auth_method is helpful here, even in the future.
I think there is usually only one method for each SCM/git provider and even if there are more, you would require extensive logic to make sure that people do not use "github authentication" when actually wanting to download from gitlab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just use
gh_api.getinstead ofrequests.getinnf_core/pipelines/download/download.py, no new flag should be needed at all. Or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vaguely recall that the problem was the need to authenticate at the API even for public repositories. For us developers, who anyway have some key-based authentication or token set up for GitHub, this is essentially unnoticeable.
But for ordinary users, who just get started with nf-core and would like to download their first pipeline, this represents a significant obstacle since they do barely understand the error message or might not even have a GitHub account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes kind of. I think it is possible to use the API unauthenticated, too but you would be rate limited more easily (which is not the case for the public zip download url, afaik). Since I don't know how many requests the CI or some power user does in a short timeframe I decided to put it behind a flag.