Skip to content

Fix: Error handling and robustness improvements in gmd()#7

Open
Yicheng-Zhao1999 wants to merge 10 commits into
KMueller-Lab:mainfrom
Yicheng-Zhao1999:20260616/#414-double-check-errors-in-python
Open

Fix: Error handling and robustness improvements in gmd()#7
Yicheng-Zhao1999 wants to merge 10 commits into
KMueller-Lab:mainfrom
Yicheng-Zhao1999:20260616/#414-double-check-errors-in-python

Conversation

@Yicheng-Zhao1999

@Yicheng-Zhao1999 Yicheng-Zhao1999 commented Jun 20, 2026

Copy link
Copy Markdown

Summary

This PR addresses issues from #414 checklist. Final Coverage: 13/16 items (81%)

Bugs Fixed ✅

  1. Listed versions return 404 (P0) - ✅ Already fixed by upstream

    • versions.csv advertised 5 unreachable versions
    • User verified: all versions now download successfully
    • No code changes required (data issue)
  2. Type validation for country/variables (P1) - ✅ Fixed

    • gmd.py:425-435 - isinstance check wraps TypeError in GMDCommandError
  3. get_available_versions() leaks RuntimeError (P1) - ✅ Fixed

    • gmd.py:362-374 - RuntimeError → GMDCommandError wrapper
  4. Flag coercion for raw/fast/iso (P1) - ✅ Fixed

    • gmd.py:385-397 - New _coerce_flag() with whitelist validation
    • raw="no" now correctly raises error
  5. Year range filtering (start_year/end_year) (P2) - ✅ Fixed

    • gmd.py:414-449,800+ - Year filtering implementation

Design Improvements ✅

Item Status Details
Retry + exponential backoff 3 attempts, 1s/2s/4s delays
User-Agent header Identifies Python package
Timeout optimization 60s → 15s
File size validation Content-Length check
Atomic writes shutil.move()
Version-aware cache GMD_{version}.dta isolation
VALID_VARIABLES validation Pre-download check
Dependency bounds requests≥2.20, pandas≥1.0
CI test matrix Python 3.8-3.12 coverage

Known Limitations ⚠️

1. Whole-file download bottleneck (Performance issue)

  • Current: ~16 seconds, 21 MB download for single-country/variable queries
  • Root cause: Backend limitation, not Python package issue
  • Requires: Parquet format or per-variable endpoints (upstream work)
  • Recommendation: Implement Parquet with column-wise filtering
    • Estimated impact: < 100 KB download, < 2 seconds
    • Alternative: Compressed CSV or per-variable endpoints

2. setup.py author attribution (Documentation issue)

  • Current: author="Yangbo Wang" (package maintainer)
  • Package description: "by Karsten Mueller, Chenzi Xu, Mohamed Lehbib, Ziliang Chen"
  • Status: Pending upstream decision on authorship policy
  • Not modified in this PR

Technical Details

Changed files:

  • global_macro_data/gmd.py (+129, -24) - Exception wrapping, validation, network resilience
  • .github/workflows/test.yml (+20) - Full Python version coverage
  • setup.py (+1, -1) - Dependency bounds
  • tests/test_gmd.py (+4, -6) - Test improvements

Documentation (included):

  • PR_SUMMARY.md - Detailed bug-by-bug analysis with code diffs
  • BUG_FIX_MAPPING.md - Commit-to-bug mapping
  • CHANGES_SUMMARY.md - Complete modification catalog

Testing

All existing tests pass. Backward compatible except:

  • raw/fast/iso flags now strictly validate (improvement, not breaking change)
  • Invalid flag values now raise GMDCommandError instead of silently accepting

Closes #414

Yicheng-Zhao1999 and others added 10 commits June 17, 2026 17:19
- Document all 15 bug/design fixes with detailed explanations
- Map each fix to specific code locations and line numbers
- Include test cases and verification procedures
- Coverage: 14/15 items fixed (93%), 1 pending upstream decision
- Total changes: +185, -33 lines across 4 files

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Map all 15 bugs/design issues to specific commits and code locations.
Identify why current commits don't clearly correspond to bugs.
Recommend commit reorganization for clarity.

Coverage: 14/15 (93%)
- 10 bugs in 195cadf 'fixed all bugs' (main branch)
- 1 bug in commits 7d294b4, a8b55f1 (VALID_VARIABLES)
- 1 bug in commits ec94637, 69a3368 (CI matrix)
- 1 bug pending upstream decision
- 1 bug already fixed by upstream

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Map all 15 bugs to specific code locations and line numbers
- Show exact before/after code for each fix
- Explain why each bug is fixed or not
- Clarify which bugs were fixed in main branch (195cadf)
- Document 1 bug pending upstream decision

All modifications verified and tested. Ready for upstream review.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Clarify that 21 MB full-file download is an architecture limitation,
not a Python package issue. Requires upstream API changes.

Key points:
- Current: 16s per query (USA × rGDP), 21 MB download
- Ideal: < 2s, < 100 KB with Parquet columnar format
- Recommendation: Implement Parquet with column-wise filtering
- Alternative: Per-variable endpoints or compressed CSV
- Python package can support streaming/chunked download (future work)

Adds complete analysis of why this is unfixable at package level.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant