Claude commited on
Commit
be12b50
·
unverified ·
1 Parent(s): b0a934c

fix: remove silent failures and redundant checks

Browse files

Critical fixes for code quality (Robert C. Martin standards):

1. Replace ignore_errors=True with explicit exception handling
- pipeline.py: staging cleanup now logs failures
- adapter.py: temp dir cleanup now logs failures
- app.py: results cleanup now logs failures

2. Fix redundant truthy check on prediction_mask
- prediction_mask is typed as Path (not Optional)
- Changed misleading `x and x.exists()` to just `x.exists()`
- Added clarifying comment about defense-in-depth

3. Fix outdated docstring in create_niivue_html
- Changed "Data URL or URL" to "Gradio file URL"
- Fixed reference from NIIVUE_JS_ON_LOAD to NIIVUE_ON_LOAD_JS

4. Update test to match new cleanup behavior
- test_pipeline_cleanup now expects rmtree without ignore_errors

All 134 tests pass, mypy strict passes.

src/stroke_deepisles_demo/data/adapter.py CHANGED
@@ -326,9 +326,12 @@ class HuggingFaceDataset:
326
 
327
  def cleanup(self) -> None:
328
  """Remove temp directory and clear cache."""
329
- if self._temp_dir and self._temp_dir.exists():
330
- shutil.rmtree(self._temp_dir, ignore_errors=True)
331
- logger.debug("Cleaned up temp directory: %s", self._temp_dir)
 
 
 
332
  self._temp_dir = None
333
  self._cached_cases.clear()
334
 
 
326
 
327
  def cleanup(self) -> None:
328
  """Remove temp directory and clear cache."""
329
+ if self._temp_dir is not None and self._temp_dir.exists():
330
+ try:
331
+ shutil.rmtree(self._temp_dir)
332
+ logger.debug("Cleaned up temp directory: %s", self._temp_dir)
333
+ except OSError as e:
334
+ logger.warning("Failed to cleanup temp directory %s: %s", self._temp_dir, e)
335
  self._temp_dir = None
336
  self._cached_cases.clear()
337
 
src/stroke_deepisles_demo/pipeline.py CHANGED
@@ -162,7 +162,10 @@ def run_pipeline_on_case(
162
 
163
  # 5. Cleanup (Optional)
164
  if cleanup_staging:
165
- shutil.rmtree(staging_root, ignore_errors=True)
 
 
 
166
 
167
  elapsed = time.time() - start_time
168
 
 
162
 
163
  # 5. Cleanup (Optional)
164
  if cleanup_staging:
165
+ try:
166
+ shutil.rmtree(staging_root)
167
+ except OSError as e:
168
+ logger.warning("Failed to cleanup staging directory %s: %s", staging_root, e)
169
 
170
  elapsed = time.time() - start_time
171
 
src/stroke_deepisles_demo/ui/app.py CHANGED
@@ -84,9 +84,13 @@ def run_segmentation(
84
  global _previous_results_dir
85
 
86
  # Clean up previous results to prevent disk accumulation on HF Spaces
87
- if _previous_results_dir and _previous_results_dir.exists():
88
- shutil.rmtree(_previous_results_dir, ignore_errors=True)
89
- logger.debug("Cleaned up previous results: %s", _previous_results_dir)
 
 
 
 
90
 
91
  logger.info("Running segmentation for %s", case_id)
92
  result = run_pipeline_on_case(
@@ -106,8 +110,10 @@ def run_segmentation(
106
  dwi_path = result.input_files["dwi"]
107
  dwi_url = nifti_to_gradio_url(dwi_path)
108
 
 
 
109
  mask_url = None
110
- if result.prediction_mask and result.prediction_mask.exists():
111
  mask_url = nifti_to_gradio_url(result.prediction_mask)
112
 
113
  niivue_html = create_niivue_html(
 
84
  global _previous_results_dir
85
 
86
  # Clean up previous results to prevent disk accumulation on HF Spaces
87
+ if _previous_results_dir is not None and _previous_results_dir.exists():
88
+ try:
89
+ shutil.rmtree(_previous_results_dir)
90
+ logger.debug("Cleaned up previous results: %s", _previous_results_dir)
91
+ except OSError as e:
92
+ # Log but don't fail - cleanup is best-effort
93
+ logger.warning("Failed to cleanup %s: %s", _previous_results_dir, e)
94
 
95
  logger.info("Running segmentation for %s", case_id)
96
  result = run_pipeline_on_case(
 
110
  dwi_path = result.input_files["dwi"]
111
  dwi_url = nifti_to_gradio_url(dwi_path)
112
 
113
+ # prediction_mask is always a valid Path from the pipeline (not Optional)
114
+ # The .exists() check is defense-in-depth only
115
  mask_url = None
116
+ if result.prediction_mask.exists():
117
  mask_url = nifti_to_gradio_url(result.prediction_mask)
118
 
119
  niivue_html = create_niivue_html(
src/stroke_deepisles_demo/ui/viewer.py CHANGED
@@ -298,14 +298,14 @@ def create_niivue_html(
298
 
299
  This function generates an HTML snippet with data attributes containing
300
  volume URLs. The actual NiiVue initialization is handled by js_on_load
301
- in the gr.HTML component (see NIIVUE_JS_ON_LOAD).
302
 
303
  IMPORTANT: Gradio's gr.HTML strips <script> tags for security.
304
  JavaScript must be passed via the js_on_load parameter instead.
305
 
306
  Args:
307
- volume_url: Data URL or URL to volume NIfTI file
308
- mask_url: Optional data URL or URL to mask NIfTI file
309
  height: Viewer height in pixels
310
 
311
  Returns:
 
298
 
299
  This function generates an HTML snippet with data attributes containing
300
  volume URLs. The actual NiiVue initialization is handled by js_on_load
301
+ in the gr.HTML component (see NIIVUE_ON_LOAD_JS).
302
 
303
  IMPORTANT: Gradio's gr.HTML strips <script> tags for security.
304
  JavaScript must be passed via the js_on_load parameter instead.
305
 
306
  Args:
307
+ volume_url: Gradio file URL (e.g., /gradio_api/file=/path/to/file.nii.gz)
308
+ mask_url: Optional Gradio file URL to mask NIfTI file
309
  height: Viewer height in pixels
310
 
311
  Returns:
tests/test_pipeline_cleanup.py CHANGED
@@ -50,4 +50,5 @@ def test_pipeline_cleanup_default(temp_dir: Path) -> None:
50
  staging_root_passed = args[1]
51
 
52
  # Verify rmtree was called with that same path
53
- mock_rmtree.assert_called_with(staging_root_passed, ignore_errors=True)
 
 
50
  staging_root_passed = args[1]
51
 
52
  # Verify rmtree was called with that same path
53
+ # Note: We no longer use ignore_errors=True; failures are logged instead
54
+ mock_rmtree.assert_called_with(staging_root_passed)