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

Gradient Boosting - Remove try/catch around imports #6584

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

PrimozGodec
Copy link
Contributor

@PrimozGodec PrimozGodec commented Sep 21, 2023

Issue

After #6570, try/catch around xgboost/catboost methods is not required anymore since packages are always installed

Description of changes

Remove try/catch around imports, skips in tests, and simplify parts of the code.

This PR also fix translations since the check is currently failing on the master.

Includes
  • Code changes
  • Tests
  • Documentation

@PrimozGodec PrimozGodec force-pushed the boosting-remove-imports branch 2 times, most recently from 130bbcf to 297ec2c Compare September 21, 2023 13:32
@markotoplak
Copy link
Member

Come on, let's wait a bit with this so that we see if the added requirements create problems for anyone. I am making this a draft.

@markotoplak markotoplak marked this pull request as draft September 21, 2023 17:05
@PrimozGodec PrimozGodec marked this pull request as ready for review January 11, 2024 16:31
@PrimozGodec PrimozGodec reopened this Jan 11, 2024
@PrimozGodec
Copy link
Contributor Author

/rebase

@PrimozGodec PrimozGodec force-pushed the boosting-remove-imports branch from 7ad0e49 to 3364ea4 Compare January 11, 2024 16:49
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Merging #6584 (637f695) into master (7233533) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 637f695 differs from pull request most recent head 360b7b3. Consider uploading reports for the commit 360b7b3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6584      +/-   ##
==========================================
- Coverage   88.07%   88.05%   -0.02%     
==========================================
  Files         322      322              
  Lines       70254    70226      -28     
==========================================
- Hits        61874    61841      -33     
- Misses       8380     8385       +5     

@PrimozGodec PrimozGodec marked this pull request as draft January 11, 2024 17:04
@PrimozGodec PrimozGodec force-pushed the boosting-remove-imports branch from 637f695 to 55f4965 Compare January 12, 2024 07:39
@PrimozGodec PrimozGodec force-pushed the boosting-remove-imports branch from dc080ec to 360b7b3 Compare January 12, 2024 07:47
@PrimozGodec PrimozGodec marked this pull request as ready for review January 12, 2024 07:50
@PrimozGodec
Copy link
Contributor Author

I think it can be reviewed and merged now.

@markotoplak
Copy link
Member

I'd wait with this a little bit now that newest xgboost version started not packing libomp again. Perhaps we should even think about dropping the requirement...

@markotoplak markotoplak marked this pull request as draft July 26, 2024 13:02
@markotoplak markotoplak removed their assignment Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants