Skip to content

extract filename from pyfive S3 Dataset correctly#197

Merged
valeriupredoi merged 6 commits into
mainfrom
extract_filename_from_Dataset
Feb 20, 2026
Merged

extract filename from pyfive S3 Dataset correctly#197
valeriupredoi merged 6 commits into
mainfrom
extract_filename_from_Dataset

Conversation

@valeriupredoi

@valeriupredoi valeriupredoi commented Feb 16, 2026

Copy link
Copy Markdown
Collaborator

Description

Dataset.id._filename currently returns None for an S3 Dataset (which is a pyfive Dataset)
Closes #199

Before you get started

Checklist

  • This pull request has a descriptive title and labels
  • This pull request has a minimal description (most was discussed in the issue, but a two-liner description is still desirable)
  • Unit tests have been added (if codecov test fails)
  • Any changed dependencies have been added or removed correctly (if need be)
  • If you are working on the documentation, please ensure the current build passes
  • All tests pass

@valeriupredoi
valeriupredoi marked this pull request as draft February 17, 2026 11:33
@valeriupredoi

valeriupredoi commented Feb 17, 2026

Copy link
Copy Markdown
Collaborator Author

converting this to Draft for now, I am not seeing anything wrong on the GHA machines, it's just the tests on my machine that don't pick up the _filename - no, I spoke with my rear, it still fails there too, it's just that that test is skipped on GHA - still keeping it Draft since I needs to fix the tests

This is the test that fails at PyActiveStorage: https://github.com/NCAS-CMS/PyActiveStorage/actions/runs/22106044823/job/63888954418

I'll clean up and fix tests here, and add a test case, tomorrow 🍺

@codecov

codecov Bot commented Feb 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.62%. Comparing base (9f9e955) to head (b5d1d62).
⚠️ Report is 82 commits behind head on main.

Files with missing lines Patch % Lines
pyfive/h5d.py 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #197      +/-   ##
==========================================
- Coverage   77.62%   77.62%   -0.01%     
==========================================
  Files          15       15              
  Lines        3124     3128       +4     
  Branches      497      499       +2     
==========================================
+ Hits         2425     2428       +3     
  Misses        573      573              
- Partials      126      127       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@valeriupredoi

Copy link
Copy Markdown
Collaborator Author

@bnlawrence @davidhassell it'd be good to have this in the sooner the better, one test fail in PyActiveStorage because of this (the test manipulating already-loaded datasets that need file names extracted from the Dataset's metadata)

@davidhassell davidhassell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi V - looks good, with one minor change.
Cheers,
David

Comment thread pyfive/h5d.py Outdated
Co-authored-by: David Hassell <davidhassell@users.noreply.github.com>
@valeriupredoi

Copy link
Copy Markdown
Collaborator Author

Hi V - looks good, with one minor change. Cheers, David

mega, thanks very much @davidhassell - could you pls Approve? 🍻

@valeriupredoi
valeriupredoi merged commit b8c2412 into main Feb 20, 2026
7 of 8 checks passed
@valeriupredoi
valeriupredoi deleted the extract_filename_from_Dataset branch February 20, 2026 15:53
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.

Dataset loaded from S3 file returns _filename as "None" (str)

2 participants