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

Move datapack from Depends to Suggests in data.land #3373

Merged
merged 6 commits into from
Sep 6, 2024

Conversation

dlebauer
Copy link
Member

@dlebauer dlebauer commented Sep 6, 2024

Description

This reverts one of the changes from #2670 moving datapack from Imports to Suggests. As recommended by @infotroph in that PR:

my advice these days is to just put things into Imports unless there's a specific reason to move them to Suggests.

A search of the codebase shows the datapack package is only used once in the data.land package, and it is used directly following a call to the dataone package in the dataone_download function

files <- datapack::getValue(pkg, name="sysmeta@formatId")

Rationale for change:

  • it is only used in one very specialized function
  • The dataone package is in Suggests so by symmetry it seems datapack should / could also be in Suggests

Motivation and Context

Proposed by BU Research Computing Services in Slack

Motivating discussion:

Hello. I am part of Research Computing Services at Boston University. I am working with a researcher that is trying to do a native install of PEcAn on our compute cluster. The instructions we are following are linked below:
https://pecanproject.github.io/pecan-documentation/v1.7.1/pecan-manual-setup.html#install-native
For step 3, when we run make, we encounter the error that the datapack R library failed to install because the redlands library is missing on our system. I looked up the source code for redlands library and it seems it hasn't been updated since 2014. Does PEcAn still use this library?
https://download.librdf.org/source/
A few months ago, I worked with another researcher, that was attempting to install the "data.land" module and at that point we also ran into the same issue. While investigating that issue I noticed that the DESCRIPTION file (linked below) for "data.land" module had marked "redlands" R package as optional and "datapack" as required. Since "datapack" requires the R "redlands" package, there is an overriding dependency happening here. So this suggests maybe redland is no longer required?
https://github.com/PecanProject/pecan/blob/develop/modules/data.land/DESCRIPTION
We did resolve the issue by making "datapack" as an optional install and it seemed that solution was satisfactory for the researcher.
So before I put in the effort to try and install redland on our cluster, I am wondering if the R package "datapack" is required for the installation of PEcAn? Or could it be marked as optional

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I will do this if implemented

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Proposed by BU Research Computing Services
@dlebauer dlebauer marked this pull request as draft September 6, 2024 18:04
@dlebauer
Copy link
Member Author

dlebauer commented Sep 6, 2024

@infotroph
Copy link
Member

dataone package is in Suggests so by symmetry it seems datapack should / could also be in Suggests

I fully endorse this logic 👍

@infotroph

This comment was marked as resolved.

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

implemented manually, should this be done w/ generate_dependencies.R?

Yes, it should. In this case your result is probably identical, but pls use the script

  • update Changelog

I think this is a modules/data.land/NEWS.md change but not a CHANGELOG.md change.

@dlebauer
Copy link
Member Author

dlebauer commented Sep 6, 2024

NEWS.md is empty for v 1.8.0.9000, which is the version in DESCRIPTION. Should I add information there?

At the repository CHANGELOG.md level, the section for adding changes is [Unreleased]. Is there an intentional discrepancy in these protocols? Or should I create a new section for Unreleased changes? Should this protocol be documented?

@github-actions github-actions bot added the Tests label Sep 6, 2024
@dlebauer dlebauer marked this pull request as ready for review September 6, 2024 21:14
@infotroph
Copy link
Member

infotroph commented Sep 6, 2024

NEWS.md is empty for v 1.8.0.9000, which is the version in DESCRIPTION. Should I add information there?

Yep, please

At the repository CHANGELOG.md level, the section for adding changes is [Unreleased]. Is there an intentional discrepancy in these protocols? Or should I create a new section for Unreleased changes? Should this protocol be documented?

[unreleased] is what we've been using in CHANGELOG for as long as I've been on the project; x.y.z.9000 is the convention I see in the NEWS files of other R packages that use the .9000 = dev version convention.

In either case it seems fairly transparent to me that in both files the basic convention is "add new changes at the top, then at release time we label them with the correct version"

Copy link
Member

@infotroph infotroph left a comment

Choose a reason for hiding this comment

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

One more piece I should have asked for to start with: Need to add an if to make the dataone_download function throw a useful error if datapack isn't installed. @dlebauer unless you object, I'll make that change and push it straight to this branch.

@dlebauer
Copy link
Member Author

dlebauer commented Sep 6, 2024

NEWS.md updated

@dlebauer
Copy link
Member Author

dlebauer commented Sep 6, 2024

fairly transparent to me

Between different filenames and conventions it may not be as clear to others - it's still not clear to me when we should update news vs changelog or both

@dlebauer
Copy link
Member Author

dlebauer commented Sep 6, 2024

add an if to make the dataone_download function throw a useful error

Go for it, thanks!

@infotroph infotroph merged commit df0710b into develop Sep 6, 2024
12 of 13 checks passed
@infotroph infotroph deleted the datapack_to_suggests branch September 6, 2024 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants