Skip to content

StaticFetcher() Implementation + from_pdb() refactor - #5436

Open
jauy123 wants to merge 110 commits into
MDAnalysis:developfrom
jauy123:Fetcher_Class
Open

StaticFetcher() Implementation + from_pdb() refactor#5436
jauy123 wants to merge 110 commits into
MDAnalysis:developfrom
jauy123:Fetcher_Class

Conversation

@jauy123

@jauy123 jauy123 commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Fixes #5429 and #5431

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.

@jauy123

jauy123 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Docs need to be reupdated, but the code itself feel like it is in a completed state.

@jauy123
jauy123 requested a review from yuxuanzhuang August 4, 2026 21:44
@read-the-docs-community

read-the-docs-community Bot commented Aug 5, 2026

Copy link
Copy Markdown

@jauy123 jauy123 mentioned this pull request Aug 9, 2026
4 tasks

@yuxuanzhuang yuxuanzhuang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the code is in pretty good shape. I will add more comments related to the code.

For the documentation, it would be helpful to add an overview page at package/doc/sphinx/source/documentation_pages/fetchers_modules.rst that includes:

  1. Usage examples.
  2. A list of the available fetchers.

See package/doc/sphinx/source/documentation_pages/converters.rst for an example.


cache_files = [
path
for path in self.cache_path.rglob("*")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Checking whether a requested file is present in the registry should not require scanning the entire cache directory. That is error-prone since unrelated cache files can also cause false failures; it also adds unnecessary I/O.

@jauy123 jauy123 Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

check_registry() might be a misleading name. This method is meant to return all files not in the self.cache_path per the docstring

Return paths relative to :attr:`cache_path` for cache files that are
missing from the registry.

I originally made it when I was refactoring the fetch() method where it would toss error to prevent unintentional update of the registry (which would be a security issue)

        if LOAD_FROM_CACHE:
            registry_dictionary = self.read_registry(db_path)
            missing_files_list = self.check_registry(
                db_path, files=list(requested_files)
            )

            if len(missing_files_list) != 0:
                MISSING_FILES = True

        if MISSING_FILES and not APPEND_DATABASE:
            raise ValueError(
                "fetch() is requesting files not found in the registry. "
                + f"The missing files are {missing_files_list}. "
                + "To fix this, please set append_db=True to append the "
                + "registry."
            )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This method could be renamed because it's not really checking like return a True or False

Comment thread .gitignore Outdated
Comment thread package/MDAnalysis/fetch/fetchers.py Outdated
Comment thread package/MDAnalysis/fetch/fetchers.py
Comment thread package/MDAnalysis/fetch/fetchers.py Outdated
Comment thread package/MDAnalysis/fetch/fetchers.py Outdated
Comment thread package/MDAnalysis/fetch/fetchers.py Outdated
jauy123 and others added 4 commits August 11, 2026 16:11
Co-authored-by: Yuxuan Zhuang <yuzhuang@stanford.edu>
Co-authored-by: Yuxuan Zhuang <yuzhuang@stanford.edu>
Co-authored-by: Yuxuan Zhuang <yuzhuang@stanford.edu>
Co-authored-by: Yuxuan Zhuang <yuzhuang@stanford.edu>
@jauy123 jauy123 mentioned this pull request Aug 12, 2026
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fetcher GSoC GSoC project

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[GSoC 2026] Implementation of Static Fetcher

5 participants