-
Notifications
You must be signed in to change notification settings - Fork 828
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
fix: Fix reflective class loading in IntelliJ #1456
Conversation
lightgbm/src/main/scala/com/microsoft/azure/synapse/ml/lightgbm/swig/SwigUtils.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THis is such an difficult problem you fixed. Just a few things for mutability reduction
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/JarLoadingUtils.scala
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/JarLoadingUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/JarLoadingUtils.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/JarLoadingUtils.scala
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #1456 +/- ##
==========================================
- Coverage 84.39% 84.31% -0.09%
==========================================
Files 284 284
Lines 14427 14439 +12
Branches 718 722 +4
==========================================
- Hits 12176 12174 -2
- Misses 2251 2265 +14
Continue to review full report at Codecov.
|
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Summary
Some tests rely on the ability to reflect all classes in the synpaseml package (e.g. to filter to a set of all classes derived from a particular parent class). The current implementation uses ClassPath.from() from a spark utility package. However, this utility only works for ClassLoaders derived from UrlClassLoader (not well documented limitation). In the context of sbt, this is true, so things work in pipelines and such. In IntelliJ, the ClassLoaders are the standard App/Ext/Boot trio, which are not, so any test using this pathway fails since ClassPath.from() returns an empty list.
This PR adds a double check so that if the reflected class list from ClassPath comes back empty, it falls back to a manual lookup. This lookup is slower, so the choice was to not replace it, but instead add a path that should only run when a test is run from inside IntelliJ. Caching was added so the lookup is only done once in either case.
Tests
Some tests that were failing before in IntelliJ now work (used first FuzzingTest as a canary). Due to the nature of the fix, there is no way to test this in the pipelines since they use sbt.
Dependency chances
N/A