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

[R-package] Move R6 to Imports #4812

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Conversation

david-cortes
Copy link
Contributor

The R package has R6 as a 'Depends', which causes it to load it fully into the namespace when loading lightgbm. This is not actually required since only one function from this package is used in the R code, and this function is prefixed with R6::. This PR moves to 'Imports' instead so that it doesn't load its full namespace in user sessions.

Also it updates the dependency on R to 3.6, since after the latest PRs it won't compile with R 3.5 anymore due to headers not having all the function prototypes.

@jameslamb jameslamb changed the title [R-package] Update dependencies [R-package] Move R6 to Imports, bump oldest supported R version to 3.6 Nov 18, 2021
Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I totally support moving R6 down to Imports, makes sense.

updates the dependency on R to 3.6, since after the latest PRs it won't compile with R 3.5 anymore due to headers not having all the function prototypes.

Can you please provide some specific evidence for this? If not, then I'd like to test this assertion before bumping this floor.

R 3.6.0 came out in April 2019 (link), and R 3.6.3 is the lowest version we tested against in this project's CI

if [[ "${R_MAJOR_VERSION}" == "3" ]]; then
export R_MAC_VERSION=3.6.3
export R_LINUX_VERSION="3.6.3-1bionic"
export R_APT_REPO="bionic-cran35/"

So if recent changes have introduced an incompatibility with R 3.5.x I'm fine increasing this floor. But I'd like to see some evidence of that before accepting this change.


Also note that I've changed the title of this pull request to be a bit more specific. PR titles in this project become release note items (see https://github.com/microsoft/LightGBM/releases/tag/v3.3.1 for an example), so we try to make them answers to the question "what would users want to know about this change?".

@david-cortes
Copy link
Contributor Author

Did a bit more research. R_FlushConsole was introduced in the R header somewhere between version 3.5.0 and 3.6.0. Don't want to find out which one specifically so I'll bump back the R version dependency.

@david-cortes david-cortes changed the title [R-package] Move R6 to Imports, bump oldest supported R version to 3.6 [R-package] Move R6 to Imports Nov 18, 2021
@jameslamb
Copy link
Collaborator

Ok thank you. I will look into the specific R versions stuff more deeply later, since you've said you're not interested in it at the moment. Thanks for bringing that to our attention!

I want to be protective of that version floor until it starts to cause maintenance problems, since I remember working in locked-down corporate environments where it was very difficult to get upgraded to newer versions of R.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

I support this change, thanks very much!

@jameslamb jameslamb merged commit 65ee8ab into microsoft:master Nov 18, 2021
@StrikerRUS StrikerRUS mentioned this pull request Jan 6, 2022
13 tasks
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants