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] factored dependency 'magrittr' out of R package #2334

Merged
merged 3 commits into from
Sep 29, 2019

Conversation

jameslamb
Copy link
Collaborator

@Laurae2 in this PR, I propose that we completely factor out the dependency magrittr, in the interest of reducing lightgbm's dependency footprint. I know magrittr itself has minimal additional dependencies, but in general I think we should strive to keep lightgbm as light as possible and I feel this particular dependency isn't providing a lot of value.

@jameslamb
Copy link
Collaborator Author

Just rebased this to master to keep up with recent changes.

@StrikerRUS
Copy link
Collaborator

@jameslamb Can we merge this? If so, I'll be able to remove magrittr dependency from #2176, which is going to be merged this weekend.

@jameslamb
Copy link
Collaborator Author

@jameslamb Can we merge this? If so, I'll be able to remove magrittr dependency from #2176, which is going to be merged this weekend.

Seems that one file is in conflict. Let me rebase and run the tests manually (because I haven't done #2335 yet).

@jameslamb
Copy link
Collaborator Author

Ok we currently have 4 failing tests:

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 33 | SKIPPED: 0 | WARNINGS: 3 | FAILED: 4 ]
1. Error: training continuation works (@test_basic.R#71)
2. Failure: Feature penalties work properly (@test_parameters.R#36)
3. Failure: Feature penalties work properly (@test_parameters.R#37)
4. Failure: Feature penalties work properly (@test_parameters.R#38)

The "training continuation" one exists on master, but I think the other three are problems I've introduced in this PR. I don't feel comfortable merging until I address those three. WIll try to do it today.

# Get corresponding feature names. Positions in split_feature_indx
# which are NA will result in an NA feature name
feature_names <- parsed_json_model$feature_names[split_feature_indx]
tree_dt[, split_feaure := feature_names]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

WOW I think this is why the tests were failing for me on this build. typo: split_feaure 😆

testing now

@jameslamb
Copy link
Collaborator Author

Ok we currently have 4 failing tests:

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 33 | SKIPPED: 0 | WARNINGS: 3 | FAILED: 4 ]
1. Error: training continuation works (@test_basic.R#71)
2. Failure: Feature penalties work properly (@test_parameters.R#36)
3. Failure: Feature penalties work properly (@test_parameters.R#37)
4. Failure: Feature penalties work properly (@test_parameters.R#38)

The "training continuation" one exists on master, but I think the other three are problems I've introduced in this PR. I don't feel comfortable merging until I address those three. WIll try to do it today.

Alright I've figured out the three test errors I introduced. R unit tests now all work. I need to fix the "training continuation works" one but this PR doesn't need to wait on that.

@Laurae2 could you take one more look at this PR and let me know if I could merge?

@StrikerRUS
Copy link
Collaborator

@jameslamb Please remove this dependency from here as well.

r-magrittr=1.5=r351h6115d3f_4 \

@jameslamb
Copy link
Collaborator Author

@jameslamb Please remove this dependency from here as well.

r-magrittr=1.5=r351h6115d3f_4 \

done. Good catch, I forgot we do that. I did a git grep magrittr just to be sure there are no other uses in non-standard places, did not find any.

@jameslamb jameslamb changed the title [R] factored dependency 'magrittr' out of R package [R-package] factored dependency 'magrittr' out of R package Sep 25, 2019
@StrikerRUS
Copy link
Collaborator

@jameslamb

done. Good catch, I forgot we do that. I did a git grep magrittr just to be sure there are no other uses in non-standard places, did not find any.

Thanks!

@jameslamb
Copy link
Collaborator Author

I just rebased this PR to master.

Tests for the R package are passing locally (see below)

export CXX=/usr/local/bin/g++-8 CC=/usr/local/bin/gcc-8
Rscript build_r.R
cd R-package/tests
Rscript testthat.R

Once all CI checks pass, I'm going to merge this.

@jameslamb jameslamb merged commit 42204c4 into microsoft:master Sep 29, 2019
@jameslamb jameslamb deleted the no_magrittr branch January 27, 2020 00:15
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
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.

3 participants