Fixes and improvements for strategies

[averissimo]

ℹ️ base branch is training, not main

See insightsengineering/nestdevs-tasks#7 for overall progress

Notable changes:

• Adds primary parent reference on each resolved dependency
  • Useful to debug which package needs to be bumped
  • TODO: present this information on the output
• Move resolve_ppm_snapshot() out of lapply local definition (re-used on Rcpp version enforcement)
• Shows build error message by {pkgdepends}
  • See screenshot below with example when [email protected] fails to compile
• Account for multiple release_dates on get_release_date()
  • This fixes problem with dun** (>= 0.1.3) that retrieves tags 0.1.3 and 0.1.3-rc1
  • Max date is used
• Support for github only dependencies (for example dun** on chev** package)
• Reduce number of calls to resolve_ppm_snapshots
• Code cleanup

How to reproduce via docker container

(assumes a valid bash and docker/podman is installed)

mkdir -p verdepcheck-sandbox/repos
cd verdepcheck-sandbox

# Checkout action
git clone -b new-strategies --depth 1 https://github.com/insightsengineering/r-verdepcheck-action

# Download bash action and `script.R`
curl -o action.bash https://gist.githubusercontent.com/averissimo/17229e87f698626ef8d2463ee80f58a7/raw/c763c2b77d078be2e14d39252f91a5ed61054de7/r-verdepcheck-action.bash
git clone -b verdepcheck_action --depth 1 https://github.com/insightsengineering/random.cdisc.data repos/random.cdisc.data

touch .Renviron # (optional) place `GITHUB_PAT=<your_github_pat>` in the file, although it's not needed
mkdir libs # cache for faster runs after first

# Run docker container
docker run -v ./r-verdepcheck-action:/workspace/r-verdepcheck-action -v ./repos:/workspace/repos -v ./action.bash:/workspace/action.bash -v ./libs:/workspace/libs -v ./.Renviron:/root/.Renviron ghcr.io/insightsengineering/rstudio_4.3.1_bioc_3.17:latest bash ./action.bash -p repos/random.cdisc.data -s min_isolated

Screenshots

[averissimo]

BLUF: When searching for release dates for non-cran refs such as teal (>= 0.10.0) it returns multiple dates (from GraphQL GH query) and a strategy to select for max date should be safe. Filtering for the actual tag doesn't seem possible/easy.

Action taken: Select for max date (it should be safe, as long as the version is specific v.#.#.# with no further "sub-versions" as v0.10 with return all tags with this prefix v0.10.0-rc@pre-release, v0.10.0 and v0.10.1)

Context: GraphQL GH query returns the 2 tags v0.10.0 and v0.10.0-rc@pre-release without any tag information.

note: Teal (>= 0.10.0) is just a public example that replicates a concrete use case.

The reproducible example below shows the problem. If we change the query for an empty string we can see that some tagged commits have Tag information and other don't (as far I know it's arbitrary with 6 out of 35 having the Tag metadata)

{
    repository(owner: "insightsengineering", name: "teal") {
      refs(refPrefix: "refs/tags/", query: "v0.10.0", first: 100) {
        nodes {
          target {
            ... on Commit {
              committedDate
              abbreviatedOid
            }
            ... on Tag {
              abbreviatedOid
              commitResourcePath
              commitUrl
              message
              name
              oid
            }
          }
        }
      }
    }
  }

Teal result 1 (query: "v0.10.0")

json output

{
  "data": {
    "repository": {
      "refs": {
        "nodes": [
          {
            "target": {
              "committedDate": "2021-10-08T15:10:35Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-10-08T15:10:35Z"
            }
          }
        ]
      }
    }
  }
} 

Teal result 2 (query: "" -- empty)

json output

{
  "data": {
    "repository": {
      "refs": {
        "nodes": [
          {
            "target": {
              "committedDate": "2021-07-01T13:15:34Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-10-08T15:10:35Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-01-27T13:02:30Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-04-07T17:37:33Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-06-09T13:07:34Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-07-29T12:16:11Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-10-14T01:06:31Z"
            }
          },
          {
            "target": {
              "abbreviatedOid": "972e2b3",
              "commitResourcePath": "/insightsengineering/teal/commit/3551e2c36d05f4d569a8385505e6ec367d0d2ef3",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/3551e2c36d05f4d569a8385505e6ec367d0d2ef3",
              "message": "inital version of teal\n",
              "name": "v0.0.1",
              "oid": "972e2b3673a1c8cc0414894ba0e67faa7e37e5e9"
            }
          },
          {
            "target": {
              "abbreviatedOid": "a250362",
              "commitResourcePath": "/insightsengineering/teal/commit/60c961856a446e310b37d2eb002c21a972d2aee4",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/60c961856a446e310b37d2eb002c21a972d2aee4",
              "message": "development version with filtering bug fix\n",
              "name": "v0.0.1.1",
              "oid": "a250362d42cc727b294d62da7fcecab8b52e7385"
            }
          },
          {
            "target": {
              "abbreviatedOid": "7315062",
              "commitResourcePath": "/insightsengineering/teal/commit/4504df76f8c8b1ca841678a1bcd4c84e2a0e7b46",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/4504df76f8c8b1ca841678a1bcd4c84e2a0e7b46",
              "message": "version 0.0.2 that works with the new kadcyla app\n",
              "name": "v0.0.2",
              "oid": "73150626634502d3209445892ea995e51fe1d868"
            }
          },
          {
            "target": {
              "abbreviatedOid": "8fcae40",
              "commitResourcePath": "/insightsengineering/teal/commit/35557f910f6d43baa9b381ee7e7b0735967b1f0e",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/35557f910f6d43baa9b381ee7e7b0735967b1f0e",
              "message": "now with hold_filtering and continue filtering\n",
              "name": "v0.0.2.1",
              "oid": "8fcae40701857fe2aba43ca9e4018a5ff6358bf6"
            }
          },
          {
            "target": {
              "abbreviatedOid": "cfcbe2d",
              "commitResourcePath": "/insightsengineering/teal/commit/080ef53aa67b402ab0ffec8798e7d05e76a934b0",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/080ef53aa67b402ab0ffec8798e7d05e76a934b0",
              "message": "Version 0.0.3\n",
              "name": "v0.0.3",
              "oid": "cfcbe2d37732457842eac812dcc082bd2cff4e8e"
            }
          },
          {
            "target": {
              "committedDate": "2018-06-09T19:16:33Z"
            }
          },
          {
            "target": {
              "committedDate": "2018-11-11T22:02:28Z"
            }
          },
          {
            "target": {
              "committedDate": "2019-02-25T21:00:05Z"
            }
          },
          {
            "target": {
              "committedDate": "2019-07-14T18:57:14Z"
            }
          },
          {
            "target": {
              "committedDate": "2019-10-11T15:40:46Z"
            }
          },
          {
            "target": {
              "committedDate": "2019-10-18T13:03:44Z"
            }
          },
          {
            "target": {
              "committedDate": "2019-12-30T11:36:31Z"
            }
          },
          {
            "target": {
              "committedDate": "2020-04-10T10:08:54Z"
            }
          },
          {
            "target": {
              "committedDate": "2020-06-15T09:24:54Z"
            }
          },
          {
            "target": {
              "committedDate": "2020-07-07T15:31:08Z"
            }
          },
          {
            "target": {
              "abbreviatedOid": "75dc075",
              "commitResourcePath": "/insightsengineering/teal/commit/eb2368d7e2a70e13fb968b0f6dc462811381ac13",
              "commitUrl": "https://github.com/insightsengineering/teal/commit/eb2368d7e2a70e13fb968b0f6dc462811381ac13",
              "message": "* `cdisc_dataset` and `dataset` now return R6 class objects (`RelationalDataset`). * A new `teal_data` function to include datasets and connectors into teal application. * `cdisc_data` function to include datasets and connectors into teal application where a `check` argument still could be used and other consistency tests are performed. * `get_raw_data` can be used to derive raw data from R6 objects e.g. (`RelationalDataset`). * `RawDatasetConnector`, `NamedDatasetConnector` and `RelationalDatasetConnector` to execute custom function call in order to get data from connection. * `CodeClass` to manage reproducibility of the data and relationships between datasets. Not directly exposed to the public interface. * `mutate_dataset` allows to modify dataset or connector via `code` argument or an R script. * `mutate_data` allows to change any dataset in `RelationalData`, `RelationalDataConnector` or `RelationalDataList`. * New wrapper functions to manipulate `RelationalDatasetConnector` and `RelationalDataset` such as `get_dataset`, `load_dataset`, `as_relational`. * New wrapper functions to manipulate `RelationalDataConnector`, `RelationalData` and `RelationalDataList` such as `get_datasets`, `load_datasets`. * `choices_labeled`, `filter_spec`, `select_spec`, `data_extract_spec`, `value_choices`,  `variable_choices` as S3 class applied on `data.frame` and also on delayed data.  * You can no longer modify the `app$datasets`, but must instead use argument `filter` in the `init` function.  * New modules were created to create a module of nested teal modules, then another one that adds the right filter pane to each tab. The `teal::init` function stays unchanged. * The `teal::init` function now returns a `UI` function with an optional `id` argument. This allows to embed it into other applications. A split view of two teal applications side-by-side is one such example and shown in a vignette. `teal::init` was turned into a wrapper function around `module_teal_with_splash.R` and developers that want to embed teal as a Shiny module should directly work with these functions (`ui_teal_with_splash` and `srv_teal_with_splash`) instead of `teal::init`. * The `teal::init` function now has a title parameter to set the title of the browser window.\t * Missing data `NA` is now explicitly addressed in the filter panel: `NA`s are excluded by default and a checkbox to include them was added. * Statistics of the data are visually depicted in terms of histograms or bar charts overlayed onto the Shiny input elements. * Added buttons to remove all filters applied to a dataset. * Restored the functionality to hide the filter panel for a module when it was constructed with `filters = NULL`. * Moved helper functions into `utils.nest` and removed unused functions `set_labels_df` and `get_labels_df`.  * `optionalSelectInput` now allows for grouped choices.  ## Refactor of FilteredData (for developers)  * `FilteredData` is now fully reactive. Now filtered data is lazy evaluated as per need. This further opens the door to bookmarking `teal` apps (bookmarking currently works for the right filtering panel, but we will make this feature more sophisticated in a future release, each module must be reviewed and adapted if it contains `reactiveValues`). * Datasets and materialized connectors are provided to `FilteredData` by `set_datasets_data` function located in `init_datasets.R` file. * Renamed `get_dataset()` method to `get_data()`. * Renamed `get_filter_call()` method to `get_filter_expr()`; returns an expression rather than a list. * Removed argument `isolate` from `get_data()` method and similar methods. You must `isolate` it yourself as needed. If you want to temporarily deactivate Shiny errors due to missing errors, you can set `options(shiny.suppressMissingContextError = TRUE)`. In general, avoid `isolate` as this breaks reactivity. * We added a development module to add several filters at once, e.g. safety filters. This is to be evaluated before it is converted into a proper module and made available to end-users.",
              "name": "v0.9.0",
              "oid": "75dc075b4c396891cbfa0314ac7fffb62476b33d"
            }
          },
          {
            "target": {
              "committedDate": "2020-10-19T12:12:30Z"
            }
          },
          {
            "target": {
              "committedDate": "2020-12-15T13:55:22Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-03-18T17:49:08Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-05-03T07:47:36Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-07-01T13:15:34Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-10-08T15:10:35Z"
            }
          },
          {
            "target": {
              "committedDate": "2021-10-08T15:10:35Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-01-27T13:02:30Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-04-07T17:37:33Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-06-09T13:07:34Z"
            }
          },
          {
            "target": {
              "committedDate": "2022-10-14T01:06:31Z"
            }
          },
          {
            "target": {
              "committedDate": "2023-05-22T18:00:53Z"
            }
          }
        ]
      }
    }
  }
} 

[pawelru]

When searching for release dates for non-cran refs such as teal (>= 0.10.0) it returns multiple dates

Very interesting finding. Checked this on my own and I do confirm.

I looked into the code to see how you addressed it and it seems that you fixed it on the consumer side to handle returned vector of dates. Wouldn't be easier / better to assure that get_release_date returns obj of length of one (in particular: NA for not found or unknown remote-ref class) - that is: handle it on the producer side? As a general note, number of consumers tends to grow over time and each of them would have to have this "fix" which is not so DRY solution.

[pawelru]

I am very interested in reasoning here. Can you please elaborate more?
I am a little concerned because this is going to be the first break of self-consistency of this object which is returned(!) (out of exported! fun) so we don't really know who and how it is used. From what I recall, sometimes we use ref element and sometimes we use other like version etc. With this change we need to be very very careful. And I would like to avoid such traps if possible

[averissimo]

The previous behaviour was breaking consistency as it changed the original ref from the remotes with the output of this function.

It replaces insightsengineering/dunlin@*release with dunlin=insightsengineering/[email protected]. (screenshot below with the error and get_resolution())

As a result, when it is trying to solve the dependencies it detects that the ...@*release ref cannot be installed.

An alternate solution would be for us to populate the version, commitish and any other field fields manually. However, I really like using pkgdepends::parse_pkg_ref for this as it reduces possible maintenance in the future.

[pawelru]

The previous behaviour was breaking consistency as it changed

It replaces

I would disagree with that. This function does not mutating or changing anything per se. It takes an object (of remore_ref class) and returns a new (!) object (of the same class). So it's like plus_one function that takes numeric and returns numeric. Note that both input and output are self-consistent because of unchanged pkgdepends::parse_pkg_ref return value as you pointed out.

The @*release ref is a bigger, more complex topic and I might not recall all the details. Please please test it very well.

First of all please note that @*release is not properly handled by pkgdepends. I opened PR but this haven't received a review so far. This can be seen in your screenshot -> formatters@*release is of 0.5.0.9003 version whereas most likely it should be 0.5.0. But this more likely refers to "release" strategy.

I struggled a lot with @*release due to conflicts. That's typical use case:
Let's assume the following:
A -imports-> B >= 1.2.3 (and has B in its Remotes with @*release)
A -imports-> C >= 1.2.3 (and has C in its Remotes with @*release)
B -imports-> C >= 1.2.3 (and has B in its Remotes with @*release)
all of A B C are internal GH packages

When tested from A perspective, we would have repo/[email protected], repo/[email protected] and recursive resolve will also add repo/c@*release. Therefore the resolution will end up with conflict. That was the main reason behind solve_ip_ignore_remotes_release. Your change will replace it with repo/b@*release and repo/c@*release so there won't be conflict anymore. (Maybe we don't need solve_ip_ignore_remotes_release anymore?) It has some side effect though being the change from repo/[email protected] into repo/c@*release that could be misleading. Obviously we have full resolution report with version etc so maybe that's not a big deal?

Let me also comment one more thing. Looking into the screenshots provided it looks like an installation error as opposed to dependency resolve conflicts! So maybe there is all is due to GH_PAT? (dunlin is a private repo)

[averissimo]

I've been thinking about this, testing and debugging and trying to understand the whole context better and design a good solution.

🗓️ I've sent an invite for us to talk on early next week (Monday?) about this.

Addressing your last point first:

it looks like an installation error as opposed to dependency resolve conflicts

It fails to install as the resolution had no candidate for the .../...@*release as the ref name was replaced by dunlin=.../[email protected].

I'm very confident that it's not a token or installation problem as the message appears to indicate.

The forced assignment after pkgdepends::parse_pkg_ref(new_ref)

⚠️ The comments bellow are only for the min_isolated strategy

I think the problem we have with Remotes is a bit wider than we realised, there are currently 3 issues (maybe more as I found a weird big also on min_cohort)

1. Your PR on support for @*release reference r-lib/pkgdepends#321
2. Can't install dependency .../...@*release
   • Proposed fix that started this discussion
3. Min version is not respected by packages that are described on Remotes dependency that also exist on CRAN
   • For example, formatters (as a primary dependency) will install 0.5.9003 instead of 0.5.0

To help me understand better the whole process I made a diagram for this strategy's technical workflow (see at the end)

💡 Idea for solution

I think the solution should be targetted when the IP is being prepared _(during first call on the diagram)

In particular, when the Need/Config/verdepcheck resolves to a particular version or to CRAN, it will modify/remove the corresponding remote.

• If it resolves to a particular tag, it changes the Remotes to that version instead of @*release. If the package is resolved to CRAN then it will remove the corresponding Remotes.
• (alternative) Solve this downstream (second call on the diagram)
  • For issue 2. using my first fix
  • For issue 3. implement logic that detects this scenario and uses a similar solution

[averissimo]

Summary:

@pawelru I've Implemented the solution we discussed on our meeting today and my initial tests show it's:

• ✅ working very well with {chevron} (pkg where some of these problems were materializing)
• ❌ The local edge case we talked about confirms our hypothesis with the encapsulation scenario
  • When a primary dependency is included in the dependency tree with a Remotes (resolving to main branch)

Here are the changes: 6079c3c4..caea5c37 -- Changes applied to all strategies, but max.

On chevron it :

• Resolves problematic package {dunlin} without any issues (installing the expected tag)
• Corrects another bug (that we didn't discuss) with {formatters}, installing the "min_isolated" version instead of the latest from main.

Local edge case ({test.repo})

(Imports: rtables, tern and averissimo/teal.logger)

averissimo/teal.logger is a non-CRAN pkg with custom Remotes that includes formatters, which is one of the primary dependencies of {test.repo}

It tries to install formatters 0.5.0.9004 instead of 0.5.0 and it validates our hypothesis with the encapsulation problem.

⚠️ note: all packages that are in CRAN will have GitHub Remotes ignored and squashed in favor of "ppm", so this example with only {tern} and {rtables} doesn't have any issues.

[averissimo]

Wouldn't be easier / better to assure that get_release_date returns obj of length of one (in particular: NA for not found or unknown remote-ref class) - that is: handle it on the producer side? As a general note, number of consumers tends to grow over time and each of them would have to have this "fix" which is not so DRY solution.

Very good point. I was doing this on the consumer side as it would make the get_ppm_snapshot_by_date() robust to unexpected parameter lengths (as it just happened)

Better to enforce the select strategy on the producer! I'll make that change.

Is it too late to include checkmate and start adding some parameter assertions to better prevent this?

[pawelru]

Is it too late to include checkmate and start adding some parameter assertions to better prevent this?

I've been thinking about that and decided to go more modest way when it comes to deps at least for the beginning until we see a really strong reason to do so. This could be one of the reasons but I don't think it's so strong to enforce it. Let's wait and check if we find more cases where it will provide value.