Freeze arguments to pick_calculator

[zulissimeta]

Summary of Changes

lru_cache is used to make sure that each MLP calculator is only instantiated once (great!). However, if you want to pass dictionaries through to the underlying calculator (say, config overrides to the fairchem calculator), you will get errors because dictionaries are mutable and lru_cache is unhappy. We can patch this by converting any dictionary kwargs to a frozendict.

Requirements

• My PR is focused on a single feature addition or bugfix.
• My PR has relevant, comprehensive unit tests.
• My PR is on a custom branch (i.e. is not named main).

Note: If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.

[Andrew-S-Rosen]

@zulissimeta: Interesting. Thank you for the PR here.

A question: what is the benefit in using frozendict as opposed to frozenset without requiring an additional dependency? For instance,

d = {"cow": "moo", "cat": "meow"}
d_frozen = frozenset(d.items()) # frozen now!
d_unfrozen = dict(d_frozen) # same as d again!

You would still need a decorator, but I haven't quite figured out yet the value-add for frozendict. If we do need frozendict though, this should be added to pyproject.toml as well.

[Andrew-S-Rosen]

Ah, frozenset will not work on a nested dict like d={"cow": {"moo": 1}}.

While not particularly elegant, I am okay with the proposed approach if there's no other option since frozendict seems reasonably well maintained. Not a huge fan of having another dependency just for this, but I don't know of a better solution. Happy to merge with a pyprojoect.toml update and your approval that it's ready.

[zulissimeta]

Thanks, think it should be good to go now.

[Andrew-S-Rosen]

@zulissimeta: Thanks! There were a number of issues left unresolved with this one; I have solved most but ask if you can resolve one remaining issue. For future PRs, if you can confirm that the CI suite runs and passes (or discuss any points where trouble remains), that would be a big help in ensuring I can merge things efficiently.

The following issues have since been resolved:

• There was a pre-commit CI error due to a commented out lint rule in pyproject.toml. I put the lint rule back.
• Most of the test suite was not running because of an incorrectly formatted GitHub Actions YAML run command. I have patched this now.

The following issue remains:

• Now that the test suite runs, there are failures in the CI because the fairchem tests were not marked to be skipped when an HF_TOKEN is not present. The test should be skipped if the HF_TOKEN is not present and if the model is not already downloaded to the cache. I will leave this to you to patch.

Additionally:

• I moved frozendict to the core pyproject.toml requirements because if a user installs fairchem-core (or any other MLP package) without relying on the optional dependency collection in pyproject.toml (e.g. pip install quacc fairchem-core), the code will crash since they did not also install frozendict, and there is currently no warning for this in the code. The option is to either use a monty.dev.requires decorator that informs the user they need to install frozendict, or we just move frozendict to the core set of dependencies. Since frozendict is currently maintained and has no dependencies itself, I have done the latter.
• With the current setup, the fairchem tests will only run on main. That is not ideal, but I agree that it is a reasonable workaround for right now. One day I I hope to have the test run on the HPC runner so it's tested on PRs, but that day is likely not today, and I don't believe there is another workaround that doesn't leak credentials.

[zulissimeta]

Thanks, think I got the test. Would be good to confirm that the fairchem test did run in the HPC test too.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.43%. Comparing base (6bc9a2d) to head (a78beac).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2695      +/-   ##
==========================================
- Coverage   97.48%   97.43%   -0.05%     
==========================================
  Files          90       90              
  Lines        3850     3859       +9     
==========================================
+ Hits         3753     3760       +7     
- Misses         97       99       +2     

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

[Andrew-S-Rosen]

Thanks, @zulissimeta! I will go ahead and merge this. As a head's up, I have not implemented the fairchem tests on the HPC runner yet. It's going to take more than a few minutes like I had anticipated. Right now, it will run on main though, which I will confirm momentarily.