Skip to content

Conversation

medha-14
Copy link
Contributor

Description

This PR is based on #5056 and contains changes that build upon it.
Once #5056 is merged, the diff for this PR will automatically update to show only the relevant changes.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@medha-14 medha-14 requested a review from a team as a code owner July 29, 2025 19:02
@medha-14
Copy link
Contributor Author

medha-14 commented Jul 29, 2025

I've updated the entry_points.py so that models can now be loaded directly from a URL. This is how it is working at the moment.

import pybamm
url = "https://raw.githubusercontent.com/medha-14/model_json/refs/heads/main/dfn.json"
model =  pybamm.Model(url = url,battery_model=pybamm.lithium_ion.BaseModel())
sim = pybamm.Simulation(model)
sim.solve([0, 3600])
sim.plot(show_plot=False)

Please let me know if any part of this approach needs changing or improvisation.

@Saransh-cpp
Copy link
Member

@medha-14 can you change the base branch here? Would be much easier to review

Comment on lines 118 to 129
def get_cache_path(url):
cache_dir = Path.home() / ".pybamm_cache" / "pybamm" / "models"
cache_dir.mkdir(parents=True, exist_ok=True)
file_hash = hashlib.md5(url.encode()).hexdigest()
return cache_dir / f"{file_hash}.json"


def clear_model_cache():
cache_dir = Path.home() / ".pybamm_cache" / "pybamm" / "models"
if cache_dir.exists():
for file in cache_dir.glob("*.json"):
file.unlink()
Copy link
Member

Choose a reason for hiding this comment

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

This should be handled using platformdirs.user_cache_dir: https://platformdirs.readthedocs.io/en/latest/api.html#cache-directory

@santacodes santacodes added the GSoC 2025 Items being done as part of GSoC 2025 label Aug 1, 2025
@agriyakhetarpal
Copy link
Member

@medha-14 can you change the base branch here? Would be much easier to review

Hi @medha-14, could you please create a PR within your fork from this link? medha-14/PyBaMM@GSoC...medha-14:PyBaMM:load_json_from_url. I see a better diff there. We could add comments on that PR until #5056 is merged. Otherwise, I think it is on the right track, modulo that we need to use platformdirs and we need tests.

Copy link

codecov bot commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.86%. Comparing base (a1aa02c) to head (c39d1c0).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #5137      +/-   ##
===========================================
- Coverage    98.88%   98.86%   -0.03%     
===========================================
  Files          320      320              
  Lines        26949    26988      +39     
===========================================
+ Hits         26648    26681      +33     
- Misses         301      307       +6     

☔ 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.

@agriyakhetarpal agriyakhetarpal marked this pull request as draft August 22, 2025 00:02
@agriyakhetarpal agriyakhetarpal changed the title [GSOC 2025] Load model JSON files from URLs [GSoC 2025] Load model JSON files from URLs Aug 27, 2025
@medha-14 medha-14 marked this pull request as ready for review August 31, 2025 09:32
from pybamm.expression_tree.operations.serialise import Serialise

APP_NAME = "pybamm"
APP_AUTHOR = "pybamm"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can keep just APP_NAME and drop APP_AUTHOR, and we can set Path(user_cache_dir(APP_NAME)) / "models" as a constant here instead.

Comment on lines +128 to +131
def get_cache_path(url: str) -> Path:
cache_dir = _get_cache_dir()
file_hash = hashlib.md5(url.encode()).hexdigest()
return cache_dir / f"{file_hash}.json"
Copy link
Member

Choose a reason for hiding this comment

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

Why MD5 and not SHA-256? :)

try:
file.unlink()
except Exception as e:
# Optional: log error instead of failing silently
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# Optional: log error instead of failing silently

Comment on lines +134 to +139
def clear_model_cache() -> None:
cache_dir = _get_cache_dir()
for file in cache_dir.glob("*.json"):
try:
file.unlink()
except Exception as e:
Copy link
Member

Choose a reason for hiding this comment

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

Note, we also store some of PyBaMM's data files in the cache dir using a pooch registry:

def get_data(self, filename: str):
"""
Fetches the data file from upstream and stores it in the local cache directory under pybamm directory.
Parameters
----------
filename : str
Name of the data file to be fetched from the registry.
Returns
-------
pathlib.PurePath
"""
self.registry.fetch(filename)
return pathlib.Path(f"{self.path}/{self.version}/{filename}")

I wonder if we could reuse some of the code here, because clearing the cache directory of JSON files could have unintended side effects. We do have JSON files there: https://github.com/pybamm-team/pybamm-data/releases/tag/v1.0.1

Could we perhaps rewrite the PR to use pooch instead? That will also provide safer defaults than using urllib.request.urlretrieve() directly, and we could rely on it for the checksum/caching bits to see if we need to download the model JSON again or not.

Comment on lines +144 to +150
def Model(
model=None,
url=None,
force_download=False,
*args,
**kwargs,
):
Copy link
Member

Choose a reason for hiding this comment

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

I think we can have better typing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GSoC 2025 Items being done as part of GSoC 2025
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants