GH-16786: Remove offset effect mojo#16787
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| int colIdx = pred1.numCols() > 1 ? 1 : 0; | ||
| long nrows = Math.min(pred1.numRows(), 100); | ||
| int differ = 0; | ||
| for (int i = 0; i < nrows; i++) { |
Check failure
Code scanning / CodeQL
Comparison of narrow type with wide type in loop condition High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix this kind of issue you ensure that the variable on the narrower side of the comparison has a type at least as wide as the other operand, or you narrow the wider operand when it is known to be safely representable in the narrower type. Here, the loop index i is int, while nrows is long. Since nrows is computed as Math.min(pred1.numRows(), 100), its value is always between 0 and 100 and thus easily fits into an int. The simplest, behavior-preserving fix is to store nrows as an int instead of a long and keep the loop as an int loop.
Concretely, in GLMMojoControlVarsOffsetTest.java, in method assertPredictionsDiffer, change the declaration of nrows on line 167 from long nrows = Math.min(...); to int nrows = (int) Math.min(...);. This yields a comparison i < nrows between two int values. Because Math.min with a long argument returns a long, we cast the result to int; this cast is safe because the maximum possible value is 100. No imports or additional methods are needed.
| @@ -164,7 +164,7 @@ | ||
|
|
||
| private void assertPredictionsDiffer(Frame pred1, Frame pred2, String label) { | ||
| int colIdx = pred1.numCols() > 1 ? 1 : 0; | ||
| long nrows = Math.min(pred1.numRows(), 100); | ||
| int nrows = (int) Math.min(pred1.numRows(), 100); | ||
| int differ = 0; | ||
| for (int i = 0; i < nrows; i++) { | ||
| if (Math.abs(pred1.vec(colIdx).at(i) - pred2.vec(colIdx).at(i)) > 1e-10) differ++; |
| Scope.track(h2oPreds); | ||
|
|
||
| // Save MOJO, reimport as GenericModel, score, and compare | ||
| File mojoFile = File.createTempFile("glm_mojo", ".zip"); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, the fix is to avoid java.io.File.createTempFile (which creates a temp file with default, possibly world-readable permissions) and instead use java.nio.file.Files.createTempFile, which on POSIX systems creates files with permissions restricted to the owner (-rw-------). Where a File object is still needed (e.g., for APIs taking File or to call deleteOnExit()), obtain it via Path.toFile() after creating the file with Files.createTempFile.
For this specific method, replace File.createTempFile("glm_mojo", ".zip") with Files.createTempFile("glm_mojo", ".zip").toFile(). This preserves the existing behavior (a real file on disk with a random name and .zip suffix) and keeps the rest of the code unchanged. We then need to import java.nio.file.Files. No other logic changes are required: deleteOnExit(), the FileOutputStream construction, and the path string usage all continue to work.
Concretely, in h2o-algos/src/test/java/hex/glm/GLMMojoControlVarsOffsetTest.java:
- Add an import for
java.nio.file.Filesnear the existing imports. - Change line 191 from
File mojoFile = File.createTempFile("glm_mojo", ".zip");toFile mojoFile = Files.createTempFile("glm_mojo", ".zip").toFile();.
| @@ -14,6 +14,7 @@ | ||
|
|
||
| import java.io.File; | ||
| import java.io.FileOutputStream; | ||
| import java.nio.file.Files; | ||
|
|
||
| import static org.junit.Assert.*; | ||
|
|
||
| @@ -188,7 +189,7 @@ | ||
| Scope.track(h2oPreds); | ||
|
|
||
| // Save MOJO, reimport as GenericModel, score, and compare | ||
| File mojoFile = File.createTempFile("glm_mojo", ".zip"); | ||
| File mojoFile = Files.createTempFile("glm_mojo", ".zip").toFile(); | ||
| mojoFile.deleteOnExit(); | ||
| try (FileOutputStream fos = new FileOutputStream(mojoFile)) { | ||
| model.getMojo().writeTo(fos); |
#16786
This will need to be rebased once #16749 is merged. Now this is based on combinations of rel-3.46.0 and #16749 so I have control variables mojo support which is in rel-3.46.0 and remove offset effect which is in #16749 but still not in rel-3.46.0.