Update JEP to 4.2.2#1332
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates JEP (Java Embedded Python) from version 4.0.3 to 4.2.2 and enhances resource management in the DeLFT integration by improving instance lifecycle handling and cleanup processes.
- Updates JEP dependency to version 4.2.2 for improved compatibility and features
- Adds proper shutdown hooks and resource cleanup for JEP thread pools
- Refactors exception handling and cleanup logic to use try-finally blocks
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| JEPThreadPoolClassifier.java | Added final modifiers, shutdown hooks, and resource cleanup methods |
| JEPThreadPool.java | Added final modifiers, shutdown hooks, and resource cleanup methods |
| DeLFTModel.java | Improved exception handling, cleanup logic, and import organization |
| DeLFTClassifierModel.java | Enhanced exception handling, cleanup patterns, and property access |
| Dockerfile.delft | Updated JEP version from 4.0.3 to 4.2.2 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
This PR updates JEP (Java Embedded Python) from version 4.0.3 to 4.2.2 and enhances the integration with DeLFT (Deep Learning Framework for Text) by improving resource management, error handling, and adding comprehensive test coverage.
- Updates JEP to version 4.2.2 in the Docker configuration
- Improves JEP resource management with proper shutdown hooks and cleanup mechanisms
- Enhances error handling and exception management in DeLFT integration
- Adds comprehensive test coverage for JEP thread pools and DeLFT models
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile.delft | Updates JEP version from 4.0.3 to 4.2.2 |
| JEPThreadPool.java | Adds shutdown hooks, proper resource cleanup, and makes fields final |
| JEPThreadPoolClassifier.java | Adds shutdown hooks, proper resource cleanup, and makes fields final |
| DeLFTModel.java | Improves error handling, cleanup logic, and code organization |
| DeLFTClassifierModel.java | Enhances error handling, cleanup logic, and GrobidProperties usage |
| JEPThreadPoolTest.java | Comprehensive test suite for JEP thread pool functionality |
| JEPThreadPoolShutdownTest.java | Separate test class for shutdown functionality |
| JEPThreadPoolClassifierTest.java | Test suite for classifier-specific thread pool |
| JEPThreadPoolClassifierShutdownTest.java | Separate test class for classifier shutdown functionality |
| JEPIntegrationTest.java | Integration tests for JEP thread pool interactions |
| DeLFTModelTest.java | Unit tests for DeLFT model functionality |
| DeLFTClassifierModelTest.java | Unit tests for DeLFT classifier model functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| Thread.currentThread().interrupt(); | ||
| try { | ||
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
This line interrupts the current thread (the main thread calling shutdown), not the executor's threads. This could cause unexpected behavior. The interrupt call should be removed here since it's in the context of shutting down the executor, not handling an interrupt.
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); |
| Thread.currentThread().interrupt(); | ||
| try { | ||
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
Similar to line 221, this interrupts the current thread calling shutdown rather than the executor's threads. This should be removed as it's not appropriate in the shutdown context.
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } catch (InterruptedException e) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| } | |
| } catch (InterruptedException e) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | ||
| try { | ||
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| } catch (InterruptedException e) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
This line interrupts the current thread (the main thread calling shutdown), not the executor's threads. This could cause unexpected behavior. The interrupt call should be removed here since it's in the context of shutting down the executor, not handling an interrupt.
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| } | |
| } catch (InterruptedException e) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| } | |
| } catch (InterruptedException e) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | ||
| try { | ||
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | ||
| executor.shutdownNow(); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
Similar to line 220, this interrupts the current thread calling shutdown rather than the executor's threads. This should be removed as it's not appropriate in the shutdown context.
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
| Thread.currentThread().interrupt(); | |
| try { | |
| if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { | |
| executor.shutdownNow(); | |
f2dfeb6 to
4b69357
Compare
This PR updates JEP to version 4.2.2 and improve a few things around the integration with DeLFT