StaticFetcher() Implementation + from_pdb() refactor by jauy123 · Pull Request #5436 · MDAnalysis/mdanalysis

jauy123 · 2026-07-14T20:15:02Z

Changes made in this Pull Request:

Implements fetch.fetchers.StaticFetcher()
Refactors fetch.pdb.from_pdb() to use StaticFetchers()

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: yes / no
From comment: #5436 (comment)

Per AI policy, I'm declaring that I used in AI in primary two ways: IDE autocomplete and help with reviewing the docstring used for the documentation to make it look more professional.

The one notable exception is that I used AI to help me draft test_multiple_downloads_existing_database and test_existing_database in testsuite/MDAnalysisTests/fetch/test_static_fetcher.py

I prompt is "Is there a unique way to determine the hash in order if two files are exactly the same including modification time in python using the standard library"

It gave code that used os.stat(), so I googled it and found pathlib.Path.stat().st_mtime in the official [docs](https://docs.python.org/3/library/pathlib.html#pathlib.Path.stat) which I then used manually for test_existing_database and test_multiple_downloads_existing_database with no AI.

PR Checklist

Issue raised/referenced?
Tests updated/added?
Documentation updated/added?
package/CHANGELOG file updated?
Is your name in package/AUTHORS? (If it is not, add it!)
LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

codecov · 2026-07-20T20:03:30Z

Codecov Report

❌ Patch coverage is 89.68254% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.84%. Comparing base (32b7808) to head (f3c45dc).
⚠️ Report is 12 commits behind head on develop.

Files with missing lines	Patch %	Lines
package/MDAnalysis/fetch/fetchers.py	88.98%	11 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #5436      +/-   ##
===========================================
- Coverage    93.85%   93.84%   -0.02%     
===========================================
  Files          182      183       +1     
  Lines        22509    22618     +109     
  Branches      3202     3221      +19     
===========================================
+ Hits         21125    21225     +100     
- Misses         922      929       +7     
- Partials       462      464       +2

☔ View full report in Codecov by Harness.
📢 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.

yuxuanzhuang

I left a few initial comments. My main concern is the cache and registry behavior, particularly for repeated fetches, pre-existing or unrelated cache files, and single- versus multiple-file inputs. Could you test these edge cases and add regression tests that confirm the intended behavior?

I likely won’t have time for another review this week, but please work on:

Get all CI checks passing. (I think old failures should get fixed once you merge to develop)
Improve test coverage (see Codecov)

yuxuanzhuang · 2026-07-21T04:21:45Z


-    downloader = pooch.create(
-        path=cache_path,
+    fetcher = StaticFetcher(cache_path=cache_path)


I think there's an issue with how cache works. If I run

> from_PDB(['9BUY']) PosixPath('~/Library/Caches/MDAnalysis_pdbs/9BUY.cif.gz') from_PDB(['3SN6']) ValueError: fetch() is requesting files not found in the registry. xxx

It's also unclear to me whether cache_path will proceed os env MDANALYSIS_FETCHER_DATA

and the error suggested setting append_db and it's not an argument for from_PDB

yuxuanzhuang · 2026-07-21T04:23:53Z

    ValueError
        For an invalid file format. Supported file formats are under Notes.

    :class:`requests.exceptions.HTTPError`


If a PDB code with no legacy PDB format download https://www.rcsb.org/structure/9BUY ; is it possible to raise some more detailed error e.g. LegacyPDBDownloadError and suggest to use cif file.

Copilot

Pull request overview

This PR introduces a new MDAnalysis.fetch.fetchers.StaticFetcher abstraction for downloading/caching files via pooch, and refactors MDAnalysis.fetch.pdb.from_PDB() to use this new fetcher as its backend.

Changes:

Adds StaticFetcher implementation with registry read/write/append helpers.
Refactors from_PDB() to delegate downloads to StaticFetcher.
Adds/updates tests and Sphinx module docs for the new fetcher module.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
testsuite/MDAnalysisTests/fetch/test_static_fetcher.py	New test coverage for `StaticFetcher` behavior and error cases.
testsuite/MDAnalysisTests/fetch/test_from_PDB.py	Updates `from_PDB()` tests for the refactored implementation.
testsuite/MDAnalysisTests/fetch/servers.py	Adds a local HTTP server helper for fetcher tests.
package/MDAnalysis/fetch/pdb.py	Refactors `from_PDB()` to use `StaticFetcher` and renames supported-format constant.
package/MDAnalysis/fetch/fetchers.py	Adds the `StaticFetcher` implementation and supporting utilities/constants.
package/MDAnalysis/fetch/init.py	Exposes `StaticFetcher` via package imports (but needs `__all__` alignment).
package/doc/sphinx/source/documentation_pages/fetchers/fetchers.rst	Adds Sphinx automodule page for `MDAnalysis.fetch.fetchers`.
package/doc/sphinx/source/documentation_pages/fetchers_modules.rst	Registers the new fetchers documentation page in the module index.
.gitignore	Adds ignores for local scripts (should be reverted/clarified).

Comments suppressed due to low confidence (2)

package/MDAnalysis/fetch/init.py:41

__all__ does not include StaticFetcher, but the module imports it, so from MDAnalysis.fetch import * will not export the new public symbol consistently.

__all__ = ["from_PDB"]

from .pdb import from_PDB
from .fetchers import StaticFetcher

package/MDAnalysis/fetch/fetchers.py:633

In _set_downloader(), when downloader="auto" and base_url doesn't match the regex (or is missing a scheme), _downloader is never set, leading to UnboundLocalError. Also, there is no default case for unsupported schemes, so the function can silently return None.

        # Regex matching if downloader is set to auto
        if downloader == "auto":
            regex = r"^([^:]+):"
            match = re.match(regex, base_url)

            if match:
                _downloader = match.group(1)

        else:
            _downloader = downloader

        match _downloader:
            case "http" | "https":
                return pooch.HTTPDownloader(**kwargs)
            case "ftp":
                return pooch.FTPDownloader(**kwargs)
            case "sftp":
                return pooch.SFTPDownloader(**kwargs)
            case "doi":
                return pooch.DOIDownloader(**kwargs)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

jauy123 · 2026-07-29T20:42:50Z

+        if APPEND_DATABASE and LOAD_FROM_CACHE:
+            self.append_registry(db_path, requested_files)
+


This is currently intentional. Should it be changed?

orbeckst · 2026-07-24T23:00:32Z

+import re
+from pathlib import Path
+from shutil import rmtree
+
+from servers import temporary_http_server
+
+import hashlib


Can you use

from .servers import temporary_http_server

?

This breaks testsuite/MDAnalysisTests/utils/test_imports.py

jauy123 · 2026-07-24T22:23:51Z

I have no idea what trigger copilot on this. I didn't turn on anything

orbeckst · 2026-07-24T22:35:42Z

@jauy123 , the copilot reviews are currently enabled to automatically occur on all PRs. I am not sure if that was intentional or just a setting changed on GitHub. If you (or anyone else) wants to jump into a discussion, I posted on discord #developers https://discord.com/channels/807348386012987462/808088023957897258/1530342254256979978

Copilot

Pull request overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (3)

testsuite/MDAnalysisTests/fetch/test_static_fetcher.py:28

from servers import ... is likely to fail under pytest because the test root is on sys.path, not this subdirectory, so servers.py may not be importable as the top-level module name servers. Import it via the MDAnalysisTests package namespace to make the test import robust.

from servers import temporary_http_server

testsuite/MDAnalysisTests/fetch/servers.py:55

The test HTTP server binds to a fixed port (7123). This can cause spurious failures when the port is already in use (or when tests run in parallel). Binding to port 0 lets the OS pick a free ephemeral port.

        server = ThreadingHTTPServer(("127.0.0.1", 7123), http_handler)

package/MDAnalysis/fetch/pdb.py:163

from_PDB(..., progressbar=...) forwards progressbar into StaticFetcher.fetch() as an unknown kwarg. StaticFetcher.fetch() currently forwards unknown kwargs into the downloader constructor, which will likely raise a TypeError (e.g., HTTPDownloader(..., progressbar=...)). Pass this through the existing verbose parameter instead.

    return fetcher.fetch(
        file_name=_pdb_ids,
        base_url="https://files.wwpdb.org/download/",
        progressbar=progressbar,
    )

orbeckst · 2026-07-24T22:48:42Z

+if HAS_POOCH:
+    import pooch
+
+REGISTRY_NAME = "hashes.txt"
+
+
+@pytest.fixture()
+def clean_up_default_cache():
+    rmtree(pooch.os_cache(DEFAULT_CACHE_NAME_DOWNLOADER), ignore_errors=True)
+    yield
+    rmtree(pooch.os_cache(DEFAULT_CACHE_NAME_DOWNLOADER), ignore_errors=True)
+


fair comment!

Indeed, a fair comment.

However, by design, this fixture exists to test the StaticFetcher().fetch(cache_path=None) argument which is guarded by requiring pooch to run the corresponding test. All the other tests uses the tmp_path fixture instead.

So the NameError should never run in this case.

jauy123 added 30 commits June 5, 2026 10:45

Added TODO

e3bc2ae

Added Pseudocode for Fetchers based on blog post

87a7b94

updated private methods

ad044ef

Added Dev Things

8fbd203

more dev stuff

c4c6b56

More Dev stuff yay

b921f53

More Dev stuff yay

f76554e

Added workflow

48e8925

Added workflow for StaticFetcher.fetch()

900ce8b

Added more notes

fc05684

Reworked to used pooch functions instead of writing own

b609fd6

Works but clunky!

93e777c

Updated TEST.py

dbd62be

Working single file fetch

f3837aa

Tests to fix for tomorrow

cefb95f

working test case

e175d6e

Working! Need to refactor

41d52be

Working! Need to refactor

1fb4857

test for gh-ci.yaml

0b8b439

Added pooch Downloader Support

3ad1d00

applied black

15e4ff9

Created Test Suite

b8de1eb

Finalized code -- test still need to be written

9312486

Made progressbar work

c4490b8

Github action working

d745faf

Repo guard removed?

7b9281d

removed duplicate imports

56ecac2

added early docs

70628d8

added header to tests

bfe74b6

Docs can build now!

000f15c

Pass by hand

89367ad

yuxuanzhuang requested changes Jul 21, 2026

View reviewed changes

updated regex

db33242

Copilot AI review requested due to automatic review settings July 24, 2026 22:02

Copilot started reviewing on behalf of jauy123 July 24, 2026 22:02 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

allowed from_pdb() to accept strings

25fe27d

Copilot AI review requested due to automatic review settings July 24, 2026 22:31

Copilot started reviewing on behalf of jauy123 July 24, 2026 22:31 View session

Copilot AI reviewed Jul 24, 2026

View reviewed changes

jauy123 added 17 commits July 28, 2026 13:22

getting test to work

8d386f3

added downloader stuff'

71b9826

added exception

d356da1

changed FILE1.txt content

2779a96

Changed content of TEST_FILE2

2bec26c

changed content of TEST_FILE_3.txt

4ed2553

fixed test_static_fetcher.py

4062c74

applied black

f3c45dc

fixed typos

e270abc

renamed test

f3d4bc8

made servers.py a relative import

eff8354

made temp server do a random port

6e89ff6

fixed mutable default arguement

c626c2a

fixed typo

dfb0568

revert local impot

4f8c8be

readded variable for backwards compatiblity

891f8c9

applied black

48e4bd3

		if APPEND_DATABASE and LOAD_FROM_CACHE:
		self.append_registry(db_path, requested_files)

Uh oh!

Conversation

jauy123 commented Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LLM / AI generated code disclosure

PR Checklist

Developers Certificate of Origin

Uh oh!

codecov Bot commented Jul 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yuxuanzhuang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jauy123 commented Jul 24, 2026

Uh oh!

orbeckst commented Jul 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jauy123 commented Jul 14, 2026 •

edited

Loading

codecov Bot commented Jul 20, 2026 •

edited

Loading