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

Can't depend on delphi.epidata in other packages #31

Closed
dajmcdon opened this issue Apr 14, 2022 · 30 comments
Closed

Can't depend on delphi.epidata in other packages #31

dajmcdon opened this issue Apr 14, 2022 · 30 comments

Comments

@dajmcdon
Copy link
Contributor

For another R package to depend on delphi.epidata, it needs two things:

  1. delphi.epidata in the DESCRIPTION (Imports/Suggests/etc)
  2. A Remotes: field in the DESCRIPTION pointing to cmu-delphi/delphi-epidata-r@dev (the repo name and the branch).

However, because the repo name differs from the package name, the installation routine will fail. This is an issue in the {remotes} package that performs the installation: r-lib/remotes#676.

Locally, one can first install delphi.epidata manually to avoid this, but any CI on Github will fail.

@brookslogan
Copy link
Contributor

@dajmcdon @dshemetov When we discussed this issue at the meeting today, I suggested it might be related to this, which was fixed by #14 (it was the .Rbuildignore, not the DESCRIPTION). Reading these again, it doesn't seem like it's the case.

Could we rename this repo to "cmu-delphi/delphi.epidata.r", or somehow forward from "cmu-delphi/delphi.epidata.r" to "cmu-delphi/delphi-epidata-r"? (I don't think we can rename the R package inside the repository to delphi-epidata-r.)

@dshemetov
Copy link
Contributor

@krivard Can I have admin privs on this repo, so I can look into redirection settings?

@brookslogan
Copy link
Contributor

Sorry, the above wouldn't work. We'd need to be working off of "cmu-delphi/delphi.epidata", or alternatively, not just rename/forward the repo, but also rename the package inside to "delphi.epidata.r".

@dshemetov
Copy link
Contributor

This feels like such a strange issue. Feels like there should be a way to install a package that has a different name from the repo.

@dajmcdon
Copy link
Contributor Author

Locally, I don't get an error. So once I uninstall delphi.epidata, running:

remotes::install_github("epiprocess")

correctly finds and installs delphi.epidata.

@dajmcdon
Copy link
Contributor Author

Here's the error from the Github action:

Error: Error: <callr_remote_error: Cannot install packages:
[49](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:49)
  * local::.: Can't install dependency cmu-delphi/epiprocess
[50](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:50)
  * cmu-delphi/epiprocess: Can't install dependency delphi.epidata
[51](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:51)
  * delphi.epidata: Can't find package called delphi.epidata.>
[52](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:52)
   in process 8[53](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:53)4 
53
  -->
[54](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:54)
  <simpleError: Cannot install packages:
[55](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:55)
  * local::.: Can't install dependency cmu-delphi/epiprocess
[56](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:56)
  * cmu-delphi/epiprocess: Can't install dependency delphi.epidata
[57](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:57)
  * delphi.epidata: Can't find package called delphi.epidata.>
[58](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:58)
  
[59](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:59)
   Stack trace:
[60](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:60)
  
[61](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:61)
   12. (function (...)  ...
[62](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:62)
   13. base:::withCallingHandlers(cli_message = function(msg) { ...
[63](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:63)
   14. get("pkg_deps_internal", asNamespace("pak"))(...)
[64](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:64)
   15. pak:::pkg_deps_internal2(pkg, upgrade, dependencies)
[65](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:65)
   16. deps$stop_for_solution_error()
[66](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:66)
   17. private$plan$stop_for_solve_error()
[67](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:67)
   18. pkgdepends:::pkgplan_stop_for_solve_error(self, private)
[68](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:68)
   19. base:::stop("Cannot install packages:\n", msg, call. = FALSE)
[69](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:69)
   20. base:::.handleSimpleError(function (e)  ...
[70](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:70)
   21. h(simpleError(msg, call))
[71](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:71)
   22. base:::stop(e)
[72](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:72)
   23. (function (e)  ...
[73](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:73)
  
[74](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:74)
   x Cannot install packages:
[75](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:75)
  * local::.: Can't install dependency cmu-delphi/epiprocess
[76](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:76)
  * cmu-delphi/epiprocess: Can't install dependency delphi.epidata
[77](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:77)
  * delphi.epidata: Can't find package called delphi.epidata. 
[78](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:78)
  
[79](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:79)
  Execution halted
[80](https://github.com/cmu-delphi/epipredict/runs/6018603382?check_suite_focus=true#step:4:80)
  Error: Process completed with exit code 1.

@dshemetov
Copy link
Contributor

Hm, these aren't very descriptive error messages. Can't be a permissions thing, since the repo is public. 🤷

@brookslogan
Copy link
Contributor

A few ideas:

  • Could this be an issue with the CI's version of devtools, remotes, pkgload, etc.? That could be resolved by installing the GitHub versions of these instead of CRAN versions (if the GitHub versions aren't already being used)?
  • We use dev as the main branch name instead of main or master; could modifying Remotes: to specify the branch help?
  • Does our modeltools install properly? If so, it'd be pretty surprising as it depends on evalcast, and both modeltools and evalcast are within cmu-delphi/covidcast. If they do work, maybe it suggests to specify the subdir in the Remotes: (if it's possible to specify the subdir to be the root of the repo)?

@krivard
Copy link
Contributor

krivard commented Apr 14, 2022

@nmdefries may have some insight on R behaving differently in a GitHub action vs a local environment

@dajmcdon
Copy link
Contributor Author

  • Not sure about the CI package versions, though the fact that Support syntax for remotes where repo name is different from package name r-lib/remotes#676 remains open suggests no.
  • I tried tagging @dev but that didn't help. (also tried delphi.epidata=cmu-delphi/delphi-epidata-r, the "full" pak specification, but the Remotes: field doesn't seem to obey that.
  • modeltools isn't under CI, so maybe never came up. evalcast is under Github CI (failing), but downloads the CRAN version of covidcast, so it's ok with that.

@dshemetov
Copy link
Contributor

Where do the CI package versions get set? If we can reproduce locally, could be easier to diagnose.

@dajmcdon
Copy link
Contributor Author

I haven't been able to reproduce it locally. It seems that pak is the R package that does the install in the ghaction (rather than remotes).

@krivard
Copy link
Contributor

krivard commented Apr 19, 2022

@dajmcdon can you paste an example failing workflow & associated YML file?

@dajmcdon
Copy link
Contributor Author

It's the R-CMD-Check here: https://github.com/cmu-delphi/epipredict/blob/main/.github/workflows/R-CMD-check.yaml

The current version of that package passes. However, in the DESCRIPTION, changing

Remotes:
  cmu-delphi/epiprocess#58

to

Remotes:
  cmu-delphi/epiprocess

fails because the current epiprocess depends on delphi.epidata.

@dajmcdon
Copy link
Contributor Author

dajmcdon commented May 6, 2022

Just an update:

I forked this repo to my account and renamed it: dajmcdon/delphi.epidata. Then making epiprocess depend on that instead of this one fixes the issue.

Is there a particular reason for this repo name?

@dshemetov
Copy link
Contributor

dshemetov commented May 9, 2022

Not that I know of. @ryantibs @krivard thoughts on renaming this repo (and likely package) to delphi.epidata?

@dajmcdon
Copy link
Contributor Author

dajmcdon commented May 9, 2022

The package is currently named delphi.epidata. And just to be clear, if you really want, you could name the repo delphi.epidata and put the package in a subdirectory named delphi-epidata-r.

@ryantibs
Copy link
Member

ryantibs commented May 9, 2022

I believe we came to the realization that i's generally easier to have one repo per package (not multiple packages in the same repo). The covidcast repo grew huge and unwieldy (three R packages, and one Python package in it). So we made two new repos for the R & Python packages:

https://github.com/cmu-delphi/delphi-epidata-r
https://github.com/cmu-delphi/delphi-epidata-py

@brookslogan
Copy link
Contributor

brookslogan commented May 10, 2022

(@dajmcdon I tested out the delphi.epidata=... form of the Remotes: for epiprocess, and for me, this made pak::pkg_install work for epiprocess and epipredict (although for the latter, for it to actually install delphi.epidata and epiprocess, epiprocess needed to be moved to the Imports: from the Suggests: of epipredict). But it also brokedevtools::install_github, as it does not accept the delphi.epidata=.... form of Remotes:. So repo renaming still seems to be the only approach to satisfy both installer packages.)

While we are discussing naming, note that epiprocess and epipredict repos do not follow the same naming convention as delphi-epidata-r; they have no -r suffix.

Collecting some options:

  • (Dan&Dmitry's suggestion:) Rename repo delphi-epidata-r to delphi.epidata. Keep delphi-epidata-py, epiprocess, epipredict as they are. The R packages consistently do not have repo or package name suffixes, the Python package repo name does. (The repo cmu-delphi/delphi-epidata-r will automatically redirect to cmu-delphi/delphi.epidata)
  • (Almost equivalent:) Set up cmu-delphi/delphi.epidata to redirect to cmu-delphi/delphi-epidata-r (by renaming delphi-epidata-r to delphi.epidata and then renaming delphi.epidata back to delphi-epidata-r). Ignore the inconsistent repo naming scheme for epiprocess, epipredict, OR rename them to have -r suffixes.
  • (Suffixes everywhere:) Rename repo delphi-epidata-r and its package to delphi.epidata.r, something similar with the python package, same with epiprocess -> epiprocess.r, epipredict -> epipredict.r.
  • (1 R + 1 Py package each repo:) Combine repos delphi-epidata-{r,py} into a repo delphi.epidata. Plan to have Python versions of epiprocess, epipredict in the same repos as their R versions as well.

Seems like approach 1 (Dan&Dmitry's suggestion) is the easiest and somewhat consistent (R repos&packages: no suffix, Python repos: -py suffix, Python packages: no suffix). The only tradeoff is potential decreases in potential-user awareness of the Python versions, which could be ameliorated by providing a link to the Python repo at the top of the R repo README. (The other approaches don't seem bad, but are more work.) [One more tradeoff: near-clobbering of the cmu-delphi/delphi-epidata repo name.]

@dajmcdon
Copy link
Contributor Author

I like approach 1 as well.

I would note that this is only an issue for packages that depend on delphi.epidata and use GitHub actions CI. This is likely a very small collection at the moment (perhaps only affecting me). And furthermore, once on CRAN, it won't matter.

So if we love the repo name, and foresee it going to CRAN soon, maybe we just leave it as is.

@brookslogan
Copy link
Contributor

Since proposing the above ideas, we noticed that cmu-delphi/delphi.epidata would be just one character away from cmu-delphi/delphi-epidata, and there is already confusion between cmu-delphi/delphi-epidata and cmu-delphi/delphi-epidata-r. This makes options 1, 2, and 4 less than ideal as stated, although some could be improved by using the name delphi.epidata.client or package name suffixes like .r or r.

@brookslogan
Copy link
Contributor

brookslogan commented Jul 15, 2022

New slate of options keeping R and Python packages separate:

Optn R repo&pkg name Python repo name Python pkg name Python module name/path
A 🟦 delphi.epidata.client delphi-epidata-client-py delphi-epidata-client delphi.epidata.client 🟡
B 🟦 delphi.epidata.client delphi-epidata-client-py delphi-epidata-client delphi_epidata_client
C delphiepidata 🟠 delphiepidata-py delphiepidata 🟠 delphiepidata 🟠
D 🟦 delphi.epidata2 delphi-epidata2-py delphi-epidata2 delphi_epidata2
E 🟦 delphi.epidata.r delphi-epidata-py delphi-epidata-py delphi_epidata_py
F delphi.epidata.r delphi-epidata-py delphi-epidata-py delphi_epidata 🔴
G 🟦 delphi.epidata.r delphi.epidata.py delphi-epidata-py delphi_epidata_py
H epidata 🔴 epidata-py epidata-py epidata 🟠
I epidatr (🔴) epidatpy epidatpy epidatpy
I' epidatr (🔴) epidatpy epidata epidata
J 🟦 depidata depidata-py depidata-py depidata
K 🟦 epidapi epidapi-py epidapi-py epidapi
L delphi.epidata 🟠 delphi-epidata-py delphi-epidata-py delphi_epidata_py
... better options?
M epidata.r 🟠 epidata-py epidata-py epidata
M' epidata.r 🟠 epidata-py epidata-py epidata_py
M'' epidata.r 🟠 epidata-py epidata epidata
N epidar (🟠) epidapy epidapy epidapy

Key:

  • 🔴 Exact name conflict with pre-existing package/module name.
  • 🟠 Close to a name conflict with our own stuff / CRAN packages (but excusing a "2" suffix on our own stuff).
  • Parens around above: conflict is with non-CRAN package by an author we know, that seems to be used only for individual (group) use.
  • 🟦 No name conflicts of the above type noted on this line.
  • 🟡 Nonstandard for single-module package, but might actually be useful if we eventually publish many Python packages/modules? Not sure how Python module system works.
  • (Note name length, consistency separately; no annotations.)

We should probably choose among the options marked 🟦. Any other opinions or better ideas? If not, I'll proceed, say, Tuesday, with implementing option F, and file an issue in the Python repo to finish off transitioning to E or G.

Some naming criteria/metrics to consider
  • (Mandatory) R package name must match R repo name. (We're trying to fix this to get CI working for dependencies.)
  • (Mandatory) R package name does not contain "-", "_".
  • (Mandatory) Python package name does not contain ".".
  • (Optional) Python package name resembles Python repo name.
  • (Optional) Python module name/path resembles Python repo name.
  • (Optional) Python repo name is similar to R one
  • (Optional) Python module name/path is similar to R one
  • (Optional(?)) Names contain "delphi"
  • (Optional) Short, easy-to-type names (I don't think any of the 🟦 options qualify on qwerty except maybe "depidata") so we don't need to library(pkgname) and potentially get a covidcast_meta conflict
  • Other name desiderata

@ryantibs
Copy link
Member

Wow this is an extremely thorough analysis! I haven't really been following but somehow I saw this pop up in my email just now.

Throwing out a suggestion not considered above (maybe you've already discussed it):

Change cmu-dephi/delphi-epidata to cmu-delphi/epidata. In my opinion, cmu-delphi/epidata is in fact a better name for that repo and I'm not sure why it was called cmu-dephi/delphi-epidata in the first place (maybe the repo delphi-epidata existed before we made the cmu-delphi org? I can't remember). It wouldn't be hard to do a redirect from cmu-delphi/delphi-epidata to cmu-delphi/epidata as well, so that the old links still work. I renamed the epiprocess package (used to be called epitools) through GitHub and it was very easy. Is there anything undesirable about this (the fact that so many users starred the delphi-epidata repo)??

Anyway, if we do that then it frees up confusion. And we could use something like row E but without the delphi. So epidata.r for the R package name and epidata-py for the Python package name. Then everything's simpler: epidata (repo for the actual API code), epidata.r (repo for the R package), epidata-py (repo for the Python package).

@brookslogan
Copy link
Contributor

Thanks, added (part of that) to the table above, option M!

  • Renaming cmu-delphi/delphi-epidata -> cmu-delphi/epidata:
    • Should keep stars
    • (Auto-redirect, so people source-ing in the client from a github raw link pointing within cmu-delphi/delphi-epidata should probably still be fine.)
    • (Stale info, should check) won't auto redirect associated GitHub Pages --- I don't really know what this entails. Is it still going to use the old pages URL with new content, or delete the old pages URL and put new content in a new location, or keep a stale copy of the old pages at the old URL + new content at the new URL? And I don't know how much effort this would be to resolve.
  • Renaming package repos to cmu-delphi/epidata{.r,-py}, etc.: only potential glitch I see here is that epidata.r is a bit close to epidata, which is an unrelated CRAN package. Skimming the CRAN policies, I don't see that this explicitly is disallowed, though. I don't see a conflict on PyPI.

Packages should be named in a way that does not conflict (irrespective of case) with any current or past CRAN package (the Archive area can be consulted), nor any current Bioconductor package. Package maintainers give the right to use that package name to CRAN when they submit, so the CRAN team may orphan a package and allow another maintainer to take it over.

  • With this and most options besides something like D involving a "2" suffix, there might be some confusion about which is the new package on PyPI.

@brookslogan
Copy link
Contributor

Thinking again, another point of concern renaming cmu-delphi/delphi-epidata is potential CI stuff (and maybe the client source) if it does not react well to redirects.

The cmu-delphi/delphi-epidata renaming could be considered separately from the v2 client package renaming.

@dajmcdon
Copy link
Contributor Author

dajmcdon commented Jul 19, 2022

My 1-cent:

  • I like @ryantibs suggestion of renaming everything to epidata with appropriate modifiers.
  • I do kinda wish there something more interesting than epidata.r, but I don't have any amazing suggestions. "J" isn't bad, but I'm not sure its added benefit is worth the cost of confusion/difficulty-of-discovery.
  • I'm on board with "E" if the epidata version doesn't persuade the powers.
  • I'm a little sad that "I" is blocked. Just to muddy the waters: epidar and epidapy. I especially like the sound of epidapy. I do think that neither of these would effectively indicate what they do. And there is already epiR to nearly conflict.

@brookslogan
Copy link
Contributor

brookslogan commented Jul 19, 2022

Katie's notes + some comments from Zoom discussion (didn't have option N at this time):

  • Preferences:
    • I --- it looks like epidatr isn't widely used and we know them; we could talk about merging/etc.
    • M'
  • Decide how (and when) to deprecate existing epidata clients in delphi-epidata/src/client
  • Consider renaming delphi-epidata to epidata-backend

@brookslogan
Copy link
Contributor

brookslogan commented Jul 26, 2022

Update:

  • I believe we are clear to use option I, but need final confirmation.
  • Potential transition process for the R package:
    • Add note to delphi.epidata README that it's being renamed / was renamed + info about what to do to update/hotfix. [Since this package is likely not in wide use, I've advertised a little bit, but without pinging, on some channels, but have not included any sort of long transitional period or deprecation warnings in the old-name package itself.]
    • Create branch delphi.epidata or old-name or similar in delphi-epidata-r holding the current client. Downstream things that break on rename could quickly reconfigure to use this branch, then fix & transition to the latest package name.
    • Pre-switch anything in production using delphi.epidata to this branch. (Tooling: we have already done in this in epiprocess by pointing to dajmcdon/delphi.epidata.) Schedule transition back to the main branch + renaming delphi.epidata -> epidatr for these things.
    • Perform the repo rename.
    • Perform the package rename on main dev (there is no main yet). [Also update the README to point to the new/additional repo name.]
    • Downstream, perform the scheduled switches to rely on main branch & epidatr package name. [And remove.packages("delphi.epidata") to prevent accidental use of the old package. Additional steps needed if developing on packages that use this (e.g., epiprocess, epipredict), or developing on epidatr itself.]

@ryantibs
Copy link
Member

Confirmed!

@brookslogan
Copy link
Contributor

brookslogan commented Aug 9, 2022

[R package rename is] complete[, and Python package rename is ready!]

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

No branches or pull requests

5 participants