You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mahout.apache.org by "vvvdwbvvv (via GitHub)" <gi...@apache.org> on 2026/03/25 08:54:39 UTC
[PR] [QDP] fix: Enhance TorchReader tests for file handling and error cases [mahout]
vvvdwbvvv opened a new pull request, #1215:
URL: https://github.com/apache/mahout/pull/1215
### Related Issues
<!-- Closes #123 -->
### Changes
- [ ] Bug fix
- [ ] New feature
- [ ] Refactoring
- [ ] Documentation
- [x] Test
- [ ] CI/CD pipeline
- [ ] Other
### Why
We already have qdp/qdp-core/tests/torch_io.rs, but the current coverage still suggests that constructor and reader state transitions in readers/torch.rs are not being exercised in the main coverage workflow.
This leaves basic reader behavior under-tested.
<!-- Why is this change needed? -->
### How
Split `tests/torch_io.rs` into two layers:
- **Unconditional tests** — cover `TorchReader::new()` (success + missing-file error), the `NotImplemented` branch on default builds, the `InvalidInput("Reader already consumed")` state, and `get_sample_size()`/`get_num_samples()` returning `None` after a failed read.
- **`#[cfg(feature = "pytorch")]` tests** — kept for real `.pt` tensor reads; tightened to also assert metadata population and second-call rejection after a successful read.
<!-- What was done? -->
### Test Result
From
```
qdp-core/src/readers/torch.rs 29 29 0.00% 4 4 0.00% 41 41 0.00% 0 0 -
```
to
```
qdp-core/src/readers/torch.rs 29 4 86.21% 4 0 100.00% 41 9 78.05% 0 0 -
```
## Checklist
- [x] Added or updated unit tests for all changes
- [ ] Added or updated documentation for all changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [QDP] fix: Enhance TorchReader tests for file handling and error cases [mahout]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1215:
URL: https://github.com/apache/mahout/pull/1215#discussion_r2987621842
##########
qdp/qdp-core/tests/torch_io.rs:
##########
@@ -14,49 +14,144 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+use qdp_core::error::MahoutError;
+use qdp_core::reader::DataReader;
+use qdp_core::readers::TorchReader;
+use std::fs;
+use std::path::PathBuf;
+use std::sync::atomic::{AtomicU64, Ordering};
+use std::time::{SystemTime, UNIX_EPOCH};
+
+fn unique_temp_path(suffix: &str) -> PathBuf {
+ static COUNTER: AtomicU64 = AtomicU64::new(0);
+
+ let nanos = SystemTime::now()
+ .duration_since(UNIX_EPOCH)
+ .unwrap()
+ .as_nanos();
+ let pid = std::process::id();
+ let counter = COUNTER.fetch_add(1, Ordering::Relaxed);
+
+ std::env::temp_dir().join(format!("qdp_torch_{pid}_{nanos}_{counter}.{suffix}"))
+}
+
+fn create_temp_file(suffix: &str) -> PathBuf {
+ let path = unique_temp_path(suffix);
+ fs::write(&path, []).unwrap();
+ path
+}
+
+#[test]
+fn test_torch_reader_new_accepts_existing_file() {
+ let path = create_temp_file("pt");
+
+ let reader = TorchReader::new(&path).unwrap();
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ fs::remove_file(path).unwrap();
+}
+
+#[test]
+fn test_torch_reader_new_rejects_missing_file() {
+ let missing_path = unique_temp_path("pt");
+
+ let err = match TorchReader::new(&missing_path) {
+ Ok(_) => panic!("expected missing-file error"),
+ Err(err) => err,
+ };
+
+ match err {
+ MahoutError::Io(message) => {
+ assert!(message.contains("PyTorch file not found"));
+ assert!(message.contains(&missing_path.display().to_string()));
+ }
+ other => panic!("expected Io error, got {other:?}"),
+ }
+}
+
+#[test]
+fn test_torch_reader_default_build_tracks_consumed_state() {
+ let path = create_temp_file("pt");
+ let mut reader = TorchReader::new(&path).unwrap();
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ let first_err = reader.read_batch().unwrap_err();
+ match first_err {
+ MahoutError::NotImplemented(message) => {
+ assert!(message.contains("PyTorch reader requires the 'pytorch' feature"));
+ }
+ other => panic!("expected NotImplemented error, got {other:?}"),
+ }
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ let second_err = reader.read_batch().unwrap_err();
+ match second_err {
+ MahoutError::InvalidInput(message) => {
+ assert_eq!(message, "Reader already consumed");
+ }
+ other => panic!("expected InvalidInput error, got {other:?}"),
+ }
+
+ fs::remove_file(path).unwrap();
+}
Review Comment:
`test_torch_reader_default_build_tracks_consumed_state` asserts the `NotImplemented` branch from `read_batch()`, which only occurs when the `pytorch` feature is *not* enabled. If the test suite is ever run with `--features pytorch`, this test will fail because the first `read_batch()` will attempt a real tensor load instead. Gate this test with `#[cfg(not(feature = "pytorch"))]` (or split into feature-specific variants) so it remains valid under both build configurations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [QDP] fix: Enhance TorchReader tests for file handling and error cases [mahout]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 commented on code in PR #1215:
URL: https://github.com/apache/mahout/pull/1215#discussion_r2987646556
##########
qdp/qdp-core/tests/torch_io.rs:
##########
@@ -14,49 +14,144 @@
// See the License for the specific language governing permissions and
// limitations under the License.
+use qdp_core::error::MahoutError;
+use qdp_core::reader::DataReader;
+use qdp_core::readers::TorchReader;
+use std::fs;
+use std::path::PathBuf;
+use std::sync::atomic::{AtomicU64, Ordering};
+use std::time::{SystemTime, UNIX_EPOCH};
+
+fn unique_temp_path(suffix: &str) -> PathBuf {
+ static COUNTER: AtomicU64 = AtomicU64::new(0);
+
+ let nanos = SystemTime::now()
+ .duration_since(UNIX_EPOCH)
+ .unwrap()
+ .as_nanos();
+ let pid = std::process::id();
+ let counter = COUNTER.fetch_add(1, Ordering::Relaxed);
+
+ std::env::temp_dir().join(format!("qdp_torch_{pid}_{nanos}_{counter}.{suffix}"))
+}
+
+fn create_temp_file(suffix: &str) -> PathBuf {
+ let path = unique_temp_path(suffix);
+ fs::write(&path, []).unwrap();
+ path
+}
+
+#[test]
+fn test_torch_reader_new_accepts_existing_file() {
+ let path = create_temp_file("pt");
+
+ let reader = TorchReader::new(&path).unwrap();
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ fs::remove_file(path).unwrap();
+}
+
+#[test]
+fn test_torch_reader_new_rejects_missing_file() {
+ let missing_path = unique_temp_path("pt");
+
+ let err = match TorchReader::new(&missing_path) {
+ Ok(_) => panic!("expected missing-file error"),
+ Err(err) => err,
+ };
+
+ match err {
+ MahoutError::Io(message) => {
+ assert!(message.contains("PyTorch file not found"));
+ assert!(message.contains(&missing_path.display().to_string()));
+ }
+ other => panic!("expected Io error, got {other:?}"),
+ }
+}
+
+#[test]
+fn test_torch_reader_default_build_tracks_consumed_state() {
+ let path = create_temp_file("pt");
+ let mut reader = TorchReader::new(&path).unwrap();
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ let first_err = reader.read_batch().unwrap_err();
+ match first_err {
+ MahoutError::NotImplemented(message) => {
+ assert!(message.contains("PyTorch reader requires the 'pytorch' feature"));
+ }
+ other => panic!("expected NotImplemented error, got {other:?}"),
+ }
+
+ assert_eq!(reader.get_num_samples(), None);
+ assert_eq!(reader.get_sample_size(), None);
+
+ let second_err = reader.read_batch().unwrap_err();
+ match second_err {
+ MahoutError::InvalidInput(message) => {
+ assert_eq!(message, "Reader already consumed");
+ }
+ other => panic!("expected InvalidInput error, got {other:?}"),
+ }
+
+ fs::remove_file(path).unwrap();
+}
Review Comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [QDP] fix: Enhance TorchReader tests for file handling and error cases [mahout]
Posted by "vvvdwbvvv (via GitHub)" <gi...@apache.org>.
vvvdwbvvv commented on PR #1215:
URL: https://github.com/apache/mahout/pull/1215#issuecomment-4131006226
@ryankert01 Updated in #58837da
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [QDP] fix: Enhance TorchReader tests for file handling and error cases [mahout]
Posted by "ryankert01 (via GitHub)" <gi...@apache.org>.
ryankert01 merged PR #1215:
URL: https://github.com/apache/mahout/pull/1215
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@mahout.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org