metrumresearchgroup / bbr

R interface for model and project management
https://metrumresearchgroup.github.io/bbr/
Other
21 stars 2 forks source link

push back minimum snapshot? #709

Closed kyleam closed 3 days ago

kyleam commented 4 days ago

[ +cc @seth127 @barrettk ]

As of the 1.11.0 release, MPN 2023-01-25 became the effective minimum snapshot. gh-681 updated the CI to reflect that.

This issue is to track the question of whether we think 2023-01-25 is too recent (https://github.com/metrumresearchgroup/bbr/pull/681#issuecomment-2194995187):

I said:

The oldest build now uses the 2023-01-25 snapshot

I think everyone is agreement that we are long overdue for a bump here. There may be some disagreement about whether 2023-01-25 is too recent. However, at this point, this is the effective minimum snapshot since the 1.11.0 release.

So my vote would be for discussion of that to not hold up merging this PR. We can create a dedicated issue to discuss whether we want to make changes to push back the minimum required snapshot.

Here are the relevant commits from gh-681:


From 5673079:

[...] several spots started using functionality not present in the
2020-06-08 snapshot, leading to bumps in the minimum versions of cli
(requires 2021-11-19 snapshot), purrr (requires 2023-01-25), and withr
(requires 2021-02-01).

Based on that, we could move the minimum snapshot to 2021-11-19 by dropping the use of purrr::list_c and purrr::list_rbind.

seth127 commented 3 days ago

thanks for posting this @kyleam

we could move the minimum snapshot to 2021-11-19 by dropping the use of purrr::list_c and purrr::list_rbind.

This seems like the most reasonable approach to me, if we're going to push things back. That gets things almost a year earlier than the snapshot 2022-08-31 snapshot on Metworx 22.09 (reference). I'm still not fully convinced this is needed though.

the question of whether we think 2023-01-25 is too recent

I do see this is a large bump, but can we articulate any practical downsides to this? The scenarios I'm seeing:

  1. User is not in a "snapshot" environment (i.e. is installing packages ad-hoc) and wants to use new bbr but has older purrr. Seems like they would just update purrr. --> Not a problem
  2. User using the pre-installed 2022-08-31 snapshot on Metworx 22.09, but wants to use the new bbr. Similar to the first, I would think that if they're willing to install a custom (newer) bbr, that they could do the same for purrr. This might depend on organizational guidelines though. --> Potential problem, but seems unlikely
  3. User is installing from a snapshot older than 2023-01-25, but wants to use the new bbr. Could be done with a custom repo for both bbr and purrr, though may not be clear to the user how to get there. --> Potential UX problem

Those are the things that come to my mind, and none of them worry me too much. The third is the most likely to be a problem, so we might just want to think more about what error they would get and how easy it would be for them to know how to get back to a working state.

That said, let me know if there's some larger issue that I'm missing entirely, or if you see other scenarios that I didn't. Thanks.

seth127 commented 3 days ago

I guess I should also note that we're weighing all of this against the value of being able to use purrr::list_c and purrr::list_rbind in our code base. These are certainly not necessary, but my understanding (mostly from @barrettk ) is that these are often useful in bbr contexts, and tend to make the code more readable (and ideally more maintainable because of this).

kyleam commented 3 days ago

I posted this issue because there was such a large, unintentional bump to something that we've been maintaining for a very long time. However, my opinion is that, now that it's done, it's not worth rolling back. That seems to align with yours

That said, let me know if there's some larger issue that I'm missing entirely, or if you see other scenarios that I didn't.

No, I think the fallout is fairly straightforward. If the user is pulling in the latest bbr from somewhere else, they'll also need to pull in a newer version of purrr (and perhaps cli and withr, depending on how old of an environment they're in).

kyleam commented 3 days ago

I said:

That seems to align with yours

Given that, I'll close this, but we can of course reopen if others chime in with objections.

seth127 commented 2 days ago

Thanks for capturing all of that here, Kyle.