add DESCRIPTION file #231

cjyetman · 2024-01-31T13:27:25Z

No description provided.

jdhoffa · 2024-01-31T13:29:07Z

Hmmm NB but this makes me a bit nervous. Are you planning to convert this into an R package?

cjyetman · 2024-01-31T13:36:32Z

Hmmm NB but this makes me a bit nervous. Are you planning to convert this into an R package?

This is towards the suggestion by @AlexAxthelm (can't remember exactly where) to use some standard R package structure even in our "workflow" repos when they contain R code

jdhoffa · 2024-01-31T13:46:11Z

Hmmm... well I would much prefer if we postpone this until after tomorrow's Dev Hangouts, where we can brainstorm and hash out this pacta.* and workflow.* conversation once and for all.

Part of the reason we made the split in the first place was that PACTA_analysis was half package and half workflow, and it made it an absolute nightmare to manage, and this feels like a big step back into that direction @AlexAxthelm

Are we cool to pause this and discuss it next DevHangouts?

See proposed next steps here: RMI/practices#2 (comment)

jdhoffa · 2024-01-31T13:47:04Z

Then, once we're all on the same page, we can proceed?

cjyetman · 2024-01-31T13:48:18Z

Are we cool to pause this and discuss it next DevHangouts?

sure, but see for example https://github.com/RMI-PACTA/workflow.pacta for this setup in use

jdhoffa · 2024-01-31T13:49:03Z

Are we cool to pause this and discuss it next DevHangouts?

sure, but see for example https://github.com/RMI-PACTA/workflow.pacta for this setup in use

I saw it there as well, and it made me nervous there as well XD

cjyetman · 2024-01-31T13:50:19Z

Oddly, renv::dependencies() returns R as a package because of the minimum version R dependency listed in DESCRIPTION, which then breaks the build script because it wants to install an R package named R. That could easily be worked around by removing the specification in DESCRIPTION.

jdhoffa · 2024-01-31T13:53:11Z

Oddly, renv::dependencies() returns R as a package because of the minimum version R dependency listed in DESCRIPTION, which then breaks the build script because it wants to install an R package named R. That could easily be worked around by removing the specification in DESCRIPTION.

I'm not against it honestly! Just lots of signs pointing towards "we should discuss and document the differences between pacta.* and workflow.* sooner than later", otherwise I fear we're going to end up with a whole zoo of repositories again

Who knows, maybe we just decide that everything we code must be an R package and leave it at that ¯_(ツ)_/¯

cjyetman · 2024-01-31T14:01:51Z

Oddly, renv::dependencies() returns R as a package because of the minimum version R dependency listed in DESCRIPTION, which then breaks the build script because it wants to install an R package named R. That could easily be worked around by removing the specification in DESCRIPTION.

opened an issue with {renv}

cjyetman · 2024-02-01T08:56:17Z

opened an issue with {renv}

wow, and it's resolved already! 😄

jdhoffa

Based on our parallel discussion:
(which was more or less "we sort of know what pacta.* is but not really what workflow.* is)

I approve this. I don't think it makes sense to be restrictive on a workflow.* since we can't even define it :-D

cjyetman · 2024-02-01T17:01:14Z

So, adding a DESCRIPTION file here breaks our Dockerfile because it uses renv::dependencies('$WORKFLOW_DIR')[['Package']] to determine what packages are needed to install. I was actually expecting that @jdhoffa and @AlexAxthelm would be very happy about that (defining the R pkg dependencies in a standardized, structured way, rather than letting renv::dependencies() magically determine them by parsing the R scripts), but ironically renv::dependencies(), when parsing a DESCRIPTION file, currently includes the minimum R version needed as an R package, which would then require parsing that out I guess. I submitted and issue with renv and it's already been fixed (😱), but not yet CRAN released and anyway I think maybe it's not a great idea to rely on having a bleeding edge version of renv to facilitate this, so I think I'm going to drop this idea (for now at least) and close this PR.

So to recap... my preferred solution would be:

use renv::dependencies('DESCRIPTION')[['Package']] in the Dockerfile to restrict the "magic" of renv to using the DESCRIPTION as the exclusive list of dependencies
but since renv::dependencies() currently (and presumably all previous versions) would require parsing out "R" as a package name, this probably isn't worth it

jdhoffa · 2024-02-01T17:12:01Z

use renv::dependencies('DESCRIPTION')[['Package']] in the Dockerfile to restrict the "magic" of renv to using the DESCRIPTION as the exclusive list of dependencies

but since renv::dependencies() currently (and presumably all previous versions) would require parsing out "R" as a package name, this probably isn't worth it

Could we not just scrap using renv, and do something like:

# Copy DESCRIPTION
COPY DESCRIPTION /workflow.transition.monitor/DESCRIPTION

# install R package dependencies
RUN Rscript -e "pak::local_install_deps(root = '/workflow.transition.monitor');"

jdhoffa · 2024-02-01T17:12:30Z

Or sorry, am I misunderstanding, and you would prefer to not do it that way, and to instead use renv?

I'm just a little confused haha

AlexAxthelm · 2024-02-01T17:15:29Z

Could we not just scrap using renv, and do something like:

NB: this is pretty much exactly what's happening with the newer Docker images i'm maintaining

https://github.com/RMI-PACTA/workflow.factset/blob/59ebf3a113a9a512de61784ea2920136c6171574/Dockerfile#L50-L56

jdhoffa · 2024-02-01T17:16:59Z

Could we not just scrap using renv, and do something like:

NB: this is pretty much exactly what's happening with the newer Docker images i'm maintaining

https://github.com/RMI-PACTA/workflow.factset/blob/59ebf3a113a9a512de61784ea2920136c6171574/Dockerfile#L50-L56

Yeah I believe that's the easiest way

cjyetman · 2024-02-01T17:18:21Z

Interesting... I tried with pak::local_deps(root = '.') (to just get the list of needed dependencies but not install them) which failed because of our "remote" GitHub packages, but pak::local_install_deps(root = '.') appears to work. I might have to experiment with that a bit to convince myself it will work properly in a different environment.

AlexAxthelm · 2024-02-01T17:20:57Z

Also note that pak looks at Remotes in DESCRIPTION, and can call from GH repos. See the one in workflow.portfolio.parsing, where I'm using that to grab pacta.portfolio.import, and the non-CRAN version of jsonvalidate.

cjyetman · 2024-02-01T17:25:48Z

At least in the current setup here, pak::local_install_deps(root = '.') is still a problem, because we

get a list of all packages required
remove our private GH hosted pkgs from that list
add our private GH hosted pkgs using parameterized tags to specific versions of those pkgs
then install the list

So if pak::local_deps(root = '.') worked as expected, we could maybe do all that with just pak and no renv, but it doesn't seem to, at least not on my machine.

cjyetman · 2024-02-01T17:27:32Z

add-DESCRIPTION > pak::local_deps()
Error:
! ! error in pak subprocess
Caused by error: 
! Could not solve package dependencies:
* local::.:
  * Can't install dependency pacta.executive.summary
  * Can't install dependency pacta.portfolio.allocate
  * Can't install dependency pacta.portfolio.audit
  * Can't install dependency pacta.portfolio.import
  * Can't install dependency pacta.portfolio.report
  * Can't install dependency pacta.portfolio.utils
* pacta.executive.summary: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
* pacta.portfolio.allocate: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
* pacta.portfolio.audit: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
* pacta.portfolio.import: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
* pacta.portfolio.report: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
* pacta.portfolio.utils: Can't find package called pacta.executive.summary, pacta.portfolio.allocate, pacta.portfolio.audit, pacta.portfolio.import, pacta.portfolio.report, pacta.portfolio.utils.
Run `rlang::last_trace()` to see where the error occurred.

cjyetman · 2024-02-01T17:28:18Z

ah, maybe I need to add the Remotes to DESCRIPTION first...

cjyetman · 2024-02-01T17:48:27Z

aargh! Docker build still fails. I think I've seen this kind of thing before... pak fails when a dependent of a dependent depends on a "remote" package. Maybe if I explicitly depend on RMI-PACTA/r2dii.colours?

…epends on it?

github-actions · 2024-02-01T18:05:52Z

Docker image from this PR (d3b7773) created

docker pull ghcr.io/rmi-pacta/workflow.transition.monitor:pr231

cjyetman · 2024-02-01T18:24:28Z

@jdhoffa and @AlexAxthelm I got this to work (finally), at least the Docker image build successfully, so could you take another close look at the diff please? After all that, I feel like I need to test the Docker image too, which I'll do offline tomorrow.

AlexAxthelm · 2024-02-01T22:06:12Z

ah, maybe I need to add the Remotes to DESCRIPTION first...

Do note that if you do this, pak grabs the HEAD from the GH repo (for us that's main). I don't know of a good way to install specific refs, without resorting to something like what we're doing in the dockerfile for this repo (specifying refs with ARG)

cjyetman · 2024-02-01T23:31:23Z

Yeah... in the Dockerfile is the only place pak::local_deps() is being used, and it strips out all the "RMI-PACTA/*" packages before adding in references to the relevant PACTA pkgs with the specific refs attached

jdhoffa · 2024-02-02T06:24:47Z

ah, maybe I need to add the Remotes to DESCRIPTION first...

Do note that if you do this, pak grabs the HEAD from the GH repo (for us that's main). I don't know of a good way to install specific refs, without resorting to something like what we're doing in the dockerfile for this repo (specifying refs with ARG)

I'm not sure how pak works (as a more-or-less superseding of devtools), but I think in devtools you could specify a branch/ commit using the usual GH syntax? I can have a look today.

jdhoffa · 2024-02-02T06:27:19Z

DESCRIPTION

+Remotes: 
+    RMI-PACTA/pacta.executive.summary, 
+    RMI-PACTA/pacta.portfolio.allocate, 
+    RMI-PACTA/pacta.portfolio.audit, 
+    RMI-PACTA/pacta.portfolio.import, 
+    RMI-PACTA/pacta.portfolio.report, 
+    RMI-PACTA/pacta.portfolio.utils


NB (more of a question really, and not something we should solve in this PR): but I wonder if it would be possible/ interesting/ useful to just add or specify our R-universe instance as a package repository? so install from CRAN or RMI R-Universe. that way we don't need to jump through weird Remotes loops, the Dockerfile just knows that anything in our R-universe is valid to install. (obviously we would still need to sort out how to specify what version to install, but I think that is doable)

That is another cool possibility with r-universe (adding r-universe as one of your CRAN mirrors), but... I'm not sure if/how it works specifying a pkg in DESCRIPTION (probably still have to use Remotes:).

Hmmm, really? I would guess that if functions as a CRAN mirror, then install.packages() would know to look there, meaning packages installed via DESCRIPTION would automatically be found without Remotes...

(I would guess that, I have no idea if that's the case)

Issue created here: RMI-PACTA/rmi-pacta.r-universe.dev#3

jdhoffa · 2024-02-02T06:28:47Z

Dockerfile

+  workflow_pkgs <- pak::local_deps(root = '$WORKFLOW_DIR')[['ref']]; \
+  workflow_pkgs <- grep('^RMI-PACTA[/]|^local::.$', workflow_pkgs, value = TRUE, invert = TRUE); \


looks awesome! Exactly what I had in mind :-)

having to add ^local::.$ to prevent it from trying to install the "local package" is a bit weird, but yeah, otherwise works pretty good

That's part of what my initial hesitancy was with adding a DESCRIPTION file to the workflow.* repo in the first place.

You are essentially stating "this is an R package", so of course most tools will think of it now as a package.

pak starts the dependency tree at the root package (every package is dependent on itself, it seems).

This PR does all the appropriate things related to adding a license to the repo, including: - adding a LICENSE file - adding a LICENSE.md file - properly specifying the license in the DESCRIPTION file depends on #231

cjyetman · 2024-02-09T01:46:46Z

Hmmm NB but this makes me a bit nervous. Are you planning to convert this into an R package?

This is towards the suggestion by @AlexAxthelm (can't remember exactly where) to use some standard R package structure even in our "workflow" repos when they contain R code

@jdhoffa I finally found the suggestion I was referring to: RMI-PACTA/workflow.data.preparation#77

add DESCRIPTION file

15391e6

cjyetman requested review from AlexAxthelm and jdhoffa January 31, 2024 13:27

cjyetman mentioned this pull request Jan 31, 2024

Add license #232

Merged

cjyetman marked this pull request as draft January 31, 2024 14:41

jdhoffa previously approved these changes Feb 1, 2024

View reviewed changes

AlexAxthelm previously approved these changes Feb 1, 2024

View reviewed changes

jdhoffa mentioned this pull request Feb 1, 2024

Scrap imports.R and use a DESCRIPTION file RMI-PACTA/workflow.prepare.pacta.indices#45

Closed

add Remotes:

c04a4fd

cjyetman dismissed AlexAxthelm’s stale review via c04a4fd February 1, 2024 17:30

cjyetman dismissed jdhoffa’s stale review via c04a4fd February 1, 2024 17:30

cjyetman added 2 commits February 1, 2024 18:36

use renv to determine R pkg dependencies from the DESCRIPTION file

ffeeec3

also exclude workflow.transition.monitor "package"

ace25c5

cjyetman added 2 commits February 1, 2024 18:49

explicitly depend on RMI-PACTA/r2dii.colours because a dependency d…

220b274

…epends on it?

use ref column from pak::local_deps()

2f58dcb

remove explicit r2dii.colours

94a1e88

jdhoffa reviewed Feb 2, 2024

View reviewed changes

jdhoffa approved these changes Feb 2, 2024

View reviewed changes

Merge branch 'main' into add-DESCRIPTION

d3b7773

cjyetman marked this pull request as ready for review February 2, 2024 07:32

cjyetman merged commit dddec2e into main Feb 2, 2024
1 check passed

cjyetman deleted the add-DESCRIPTION branch February 2, 2024 07:32

jdhoffa mentioned this pull request Feb 2, 2024

Assess if adding an R-Universe mirror allows a Dockerfile + DESCRIPTION install to function without Remotes RMI-PACTA/rmi-pacta.r-universe.dev#3

Closed

This was referenced Feb 7, 2024

add a DESCRIPTION file RMI-PACTA/workflow.data.preparation#83

Merged

package install in Dockerfile fails because it cannot find package "R" RMI-PACTA/workflow.data.preparation#96

Closed

		workflow_pkgs <- pak::local_deps(root = '$WORKFLOW_DIR')[['ref']]; \
		workflow_pkgs <- grep('^RMI-PACTA[/]\|^local::.$', workflow_pkgs, value = TRUE, invert = TRUE); \

add DESCRIPTION file #231

add DESCRIPTION file #231

Conversation

cjyetman commented Jan 31, 2024

jdhoffa commented Jan 31, 2024

cjyetman commented Jan 31, 2024 • edited Loading

jdhoffa commented Jan 31, 2024 • edited Loading

jdhoffa commented Jan 31, 2024

cjyetman commented Jan 31, 2024

jdhoffa commented Jan 31, 2024

cjyetman commented Jan 31, 2024

jdhoffa commented Jan 31, 2024 • edited Loading

cjyetman commented Jan 31, 2024

cjyetman commented Feb 1, 2024

jdhoffa left a comment

Choose a reason for hiding this comment

cjyetman commented Feb 1, 2024

jdhoffa commented Feb 1, 2024

jdhoffa commented Feb 1, 2024

AlexAxthelm commented Feb 1, 2024

jdhoffa commented Feb 1, 2024

cjyetman commented Feb 1, 2024

AlexAxthelm commented Feb 1, 2024

cjyetman commented Feb 1, 2024

cjyetman commented Feb 1, 2024

cjyetman commented Feb 1, 2024

cjyetman commented Feb 1, 2024

github-actions bot commented Feb 1, 2024 • edited Loading

cjyetman commented Feb 1, 2024

AlexAxthelm commented Feb 1, 2024

cjyetman commented Feb 1, 2024

jdhoffa commented Feb 2, 2024

jdhoffa Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

cjyetman Feb 2, 2024

Choose a reason for hiding this comment

jdhoffa Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

jdhoffa Feb 2, 2024

Choose a reason for hiding this comment

jdhoffa Feb 2, 2024

Choose a reason for hiding this comment

cjyetman Feb 2, 2024

Choose a reason for hiding this comment

jdhoffa Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

AlexAxthelm Feb 2, 2024

Choose a reason for hiding this comment

cjyetman commented Feb 9, 2024

cjyetman commented Jan 31, 2024 •

edited

Loading

jdhoffa commented Jan 31, 2024 •

edited

Loading

jdhoffa commented Jan 31, 2024 •

edited

Loading

github-actions bot commented Feb 1, 2024 •

edited

Loading

jdhoffa Feb 2, 2024 •

edited

Loading

jdhoffa Feb 2, 2024 •

edited

Loading

jdhoffa Feb 2, 2024 •

edited

Loading