Rarr - Githubissues

grimbough commented 1 year ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Repository: https://github.com/grimbough/Rarr

Confirm the following by editing each check box to '[x]'

[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
[x] I am familiar with the Bioconductor code of conduct and agree to abide by it.

I am familiar with the essential aspects of Bioconductor software management, including:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Git (optionally via GitHub).

For questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.

bioc-issue-bot commented 1 year ago

Hi @grimbough

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: Rarr
Title: Read Zarr Files in R
Version: 0.99.0
Authors@R: c(person("Mike", "Smith", 
      role=c("aut", "cre"), 
      email = "grimbough@gmail.com",
      comment = c(ORCID = "0000-0002-7800-3848"))
  )
Description: The Zarr specification defines a format for chunked, compressed, 
  N-dimensional arrays.  It's design allows efficient access to subsets of the
  stored array, and supports both local and cloud storage systems.
  Rarr aims to implement this specifcation in R with 
  minimal reliance on an external tools or libraries.
License: MIT + file LICENSE
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Imports: jsonlite, httr2, aws.s3, stringr, R.utils, utils
Suggests: 
    BiocStyle,
    covr,
    knitr,
    tinytest
VignetteBuilder: knitr
SystemRequirements: GNU make
biocViews: DataImport

vjcitn commented 1 year ago

The vignette throws

> s3_address <- "https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0 ..." ... [TRUNCATED] 

> zarr_overview(s3_address)
List of 3
 $ Code     : chr "InvalidAccessKeyId"

Temporary? Can this be made more resilient?

grimbough commented 1 year ago

That's not an issue I've encountered in testing. The Zarr file is an exemplar OME-NGFF image stack, specifically made available for testing, and shouldn't require any authentication to access it.

I doubt it's temporary, seems more likely to be a setup issue I haven't encountered. I use the aws.s3 package to access S3, which has quite a number of ways to provide AWS credentials. Perhaps something on the system you're using is conflicting in a way I've not seen on my own setup. Do you have anything set for the AWS_ACCESS_KEY_ID environment variable?

Does the example further on in the vignette, in the section "Using credentials to access private S3 buckets" work any better? That's on a different host and does require authentication to be set.

LiNk-NY commented 1 year ago

FWIW I am also getting this 403 hang up:

library(Rarr)
s3_address <- "https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0076A/10501752.zarr/0"
zarr_overview(s3_address)
#> Client error: (403) Forbidden
#> List of 3
#>  $ Code     : chr "InvalidAccessKeyId"
#>  $ RequestId: chr "tx00000000000000301d714-0063e57bcf-16699797-default"
#>  $ HostId   : chr "16699797-default-default"
#>  - attr(*, "headers")=List of 6
#>   ..$ content-length  : chr "199"
#>   ..$ x-amz-request-id: chr "tx00000000000000301d714-0063e57bcf-16699797-default"
#>   ..$ accept-ranges   : chr "bytes"
#>   ..$ content-type    : chr "application/xml"
#>   ..$ date            : chr "Thu, 09 Feb 2023 23:03:43 GMT"
#>   ..$ connection      : chr "Keep-Alive"
#>   ..- attr(*, "class")= chr [1:2] "insensitive" "list"
#>  - attr(*, "class")= chr "aws_error"
#>  - attr(*, "request_canonical")= chr "GET\n/idr/zarr/v0.4/idr0076A/10501752.zarr/0/.zarray\n\nhost:uk1s3.embassy.ebi.ac.uk\nx-amz-date:20230209T23034"| __truncated__
#>  - attr(*, "request_string_to_sign")= chr "[removed]"| __truncated__
#>  - attr(*, "request_signature")= chr "[removed]"| __truncated__
#> NULL
#> Error in parse_aws_s3_response(r, Sig, verbose = verbose): Forbidden (HTTP 403).

^{Created on 2023-02-09 with reprex v2.0.2}

Even with the OSN resource located at:

s3_address <- "https://mghp.osn.xsede.org/bir190004-bucket01/TMA11/zarr/1.zarr/0.0.0"
zarr_overview(s3_address)

grimbough commented 1 year ago

Thanks for confirming. I'm able to replicate this behaviour if I set some AWS credentials, and then try to access a public bucket. It seems that there's no easy way to temporarily override them. Passing empty strings for the key and secret to the aws.s3 functions has no effect - in fact the code explicitly ignores them and continues to look for credentials in other places.

A solution that works for me is to unset the enviroment variables:

Sys.setenv(
  "AWS_ACCESS_KEY_ID" = "",
  "AWS_SECRET_ACCESS_KEY" = ""
)

However, if you have AWS credentials stored elsewhere e.g. in ~/.aws/credentials then that will still take precedence and it'll fail.

I believe a solution to this was requested a long time ago at https://github.com/cloudyr/aws.signature/issues/61 and it doesn't look like it got any traction. I'll have a think about how to proceed. If anyone is aware of a different package for managing AWS authentication then happy to switch.

bioc-issue-bot commented 1 year ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 0a350596bdd2db1497887834b673bc6960448b0b

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 11ee72eb3405c9e09a90a7426aca9024d91b0519

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

vjcitn commented 1 year ago

A comment about naming came up. Rarr and rawrr are very close. I am not concerned about a risk of confusion but wanted to be sure this was surfaced.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 069a3298ec899f7ab868ff5de2c911cde5021bd6

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: b4b2c13b48e0504c08add6da5a0cab563b84a3fd

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 467b711220347395bc0b17ef7d3693159879324e

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

grimbough commented 1 year ago

Hi @Kayla-Morrell

I've managed to get rid of the errors and warnings, and also successfully made the package compatible with other Zarr implementations (https://github.com/joshmoore/zarr_implementations/pull/2). I'll stop making changes for now until you've managed to do a review.

Looking forward to your feedback!

Kayla-Morrell commented 1 year ago

@grimbough - Thank you for submitting to Bioconductor. Please see the initial review of the package below. The required changes must be made while the suggested changes do not have to be (though we strongly encourage them). Comment back here with updates that have been made and when the package is ready for a re-review. Also, keep in mind that the deadline for packages to be accepted for the upcoming release is April 19th.

DESCRIPTION

[ ] SUGGESTION: We encourage including the 'BugReports:' field with the relevant link to GitHub for reporting Issues.

Data

[ ] REQUIRED: There needs to be documentation of some kind for the data in the inst/extdata/ directory. The documentation should be in an inst/script/ directory and the scripts can vary. They must clearly state how the data was generated and source information. It should include source URLs and any key information regarding filtering or processing. It can be executable code, sudo code, or a text description. Ideally users should be able to download and roughly reproduce the file or object that is present as data.

Man pages

[ ] REQUIRED: There should be runnable examples in man pages that document exported objects. None of the use_* functions have runnable examples.

R code

[ ] SUGGESTION: Avoid 'cat' and 'print' outside of 'show' methods
[ ] SUGGESTION: For formating reasons, consider shorter lines. There are 68 lines that are > 80 characters long.
[ ] SUGGESTION: For formating reasons, consider multiples of 4 spaces for line indents. There are 524 lines that are not.

Best, Kayla

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: cc7d1b865c3b0f4529f0b366749018aa5f18f711

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/Rarr to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

grimbough commented 1 year ago

Hi @Kayla-Morrell thanks a lot for the review, that really helpful. I've made the following changes.

DESCRIPTION

[x] SUGGESTION: We encourage including the 'BugReports:' field with the relevant link to GitHub for reporting Issues.

I've added both a 'URL' and 'BugReports' filed to the description which link to the package's GitHub page.

Data

[x] REQUIRED: There needs to be documentation of some kind for the data in the inst/extdata/ directory. The documentation should be in an inst/script/ directory and the scripts can vary. They must clearly state how the data was generated and source information. It should include source URLs and any key information regarding filtering or processing. It can be executable code, sudo code, or a text description. Ideally users should be able to download and roughly reproduce the file or object that is present as data.

The data generation script existed in inst/docker but it wasn't documented. I've now mode the script and Dockerfile to inst/scripts and added a README with a description of the example files and details of how someone could generate on their own system.

Man pages

[x] REQUIRED: There should be runnable examples in man pages that document exported objects. None of the use_* functions have runnable examples.

I've added some example code to the man pages for the use_* functions.

R code

[x] SUGGESTION: Avoid 'cat' and 'print' outside of 'show' methods

I've replaced the calls to cat with message.

[ ] SUGGESTION: For formating reasons, consider shorter lines. There are 68 lines that are > 80 characters long.

I've made an effort to reduce some of the line lengths. Not everything has changed, but it's down to 44 lines. In some cases I thought it made readability worse by forcing line breaks, and some of the long lines are in Rd files that are automatically generated by Roxygen.

[ ] SUGGESTION: For formating reasons, consider multiples of 4 spaces for line indents. There are 524 lines that are not.

I'm happy with the indentation provided by Rstudio, which aligns against brackets rather than multiples of 4, and so haven't changed this.

I'll chase down why there's now a warning from a man page. I don't see this on my local system.

grimbough commented 1 year ago

I think the current warning is a false positive introduced by some recent changes to BiocCheck (https://github.com/Bioconductor/BiocCheck/issues/189). I'd appreciate if you could look at the package updates assuming the warning will go away, and I'll make changes to the package if @LiNk-NY points out I'm mistaken.

Kayla-Morrell commented 1 year ago

@grimbough - Thank you for making the necessary changes. I've looked everything over and I'm more than happy to accept the package. I don't see the WARNING as an issue because I assume it's on our end.

Best, Kayla

bioc-issue-bot commented 1 year ago

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.

grimbough commented 1 year ago

That's awesome. Thanks @Kayla-Morrell for taking the time to do the review.

lshep commented 1 year ago

The default branch of your GitHub repository has been added to Bioconductor's git repository as branch devel.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/grimbough.keys is not empty), then no further steps are required. Otherwise, do the following:

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("Rarr"). The package 'landing page' will be created at

https://bioconductor.org/packages/Rarr

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

Bioconductor / Contributions

Rarr #2925

DESCRIPTION

Data

Man pages

R code

DESCRIPTION

Data

Man pages

R code