Clean up var names in unit tests, avoid MP API access in GitHub workflow

[DanielYang59]

Summary

• Clean up var names in unit tests, make module level var capital and remove a few unused vars
• Avoid fetching structure from MP database as API key cannot be passed to actions triggered by forks

[DanielYang59]

I believe I have seen this error in another PR or somewhere:

File "/opt/hostedtoolcache/Python/3.10.15/x64/lib/python3.10/site-packages/mp_api/client/core/client.py", line 1031, in _submit_request_and_process
    raise MPRestError(
mp_api.client.core.client.MPRestError: REST query returned with error status code 401 on URL https://api.materialsproject.org/materials/core/?deprecated=False&_fields=structure&material_ids=mp-19017&_limit=1000 with message:
Response {
  "message":"No API key found in request"
}
Error: Process completed with exit code 1.

Looks like MP API key got lost sometimes?

[janosh]

i can't reproduce the error locally. just tried with the latest version mp-api==0.42.2. the repo definitely has the secret

gh secret list
NAME        UPDATED          
MP_API_KEY  about 24 days ago
PYPI_TOKEN  about 1 year ago

and it's passed correctly

pymatviz/.github/workflows/test.yml

Line 59 in 7e32978

MP_API_KEY: ${{ secrets.MP_API_KEY }}

[DanielYang59]

Yep that occurs intermittently, not sure what the reason is. I didn't remember where I saw this last time, would post it here if I find that.

[janosh]

@DanielYang59 thanks for debugging. agreed that MP_API_KEY should not be read at module import time but during MPRester instance creation. pinging @tsmathis and @esoteric-ephemera in case you want to consider changing

not sure that's the root cause here though. MP_API_KEY should be available before python is invoked

- name: Run script
  run: python ${{ matrix.script }}
  env:
    MP_API_KEY: ${{ secrets.MP_API_KEY }}

[DanielYang59]

agreed that MP_API_KEY should not be read at module import time but during MPRester instance creation.

I might open an issue there for easier tracking

[DanielYang59]

@janosh can you have another look at the MP_API_KEY secret in our repo (I don't have access to that)? It looks like being empty?

RuntimeError: mp api key is empty

[janosh]

i can't see it either. once you set a secret you can only change it's value, not see the current value. i just reset the secret to what i think it was before so let's see if that makes a difference

[DanielYang59]

Still no luck. I'm not seeing the test.yml or the template from your workflows repo being changed at all recently, so super confused what is causing this failure.

I would have another look tomorrow because I don't have my laptop with me ATM (which I usually use to code), so everything feels very awkward :)

Another thread I haven't got to look at, not sure if it's related at all: Github Actions reusable workflow doesn't recognize secrets #69082

[janosh]

not sure if it's related at all

don't think so, the test-scripts job doesn't have a reusable workflow

[DanielYang59]

I believe it's related to secrets access from forked repos:

With the exception of GITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository.

As you might have noticed both this PR and #210 were created from our forks.

However above documentation doesn't provide any workaround for this :(

Perhaps skip that particular script (structure_viz.py) when running from fork?

[janosh]

that sounds like it could be the culprit. probably the thing to do is to rather than fetch those structures on the fly, save them as JSON to tests/files/structs and load them from there in this script (while leaving the code that generated those structures files in place but behind if no os.path.isfile(...) as documentation)

[DanielYang59]

You always have helpful suggestions :) How is 95614f2 looking? I don't really like to manually download and save files or such, it's always better to automate this

[DanielYang59]

Notice another issue, both our test suite and pre-commit seems to target main branch in this PR's for some reason.

At commit 1f11e0b, main branch has NOT been merged (test_structure_viz.py has a total of 224 lines) but errors are reported on recently included change from main branch that hasn't yet been merged.

---

Update: Looks like something wrong with the "checkout" action perhaps actions/checkout#1359 leads to the following:

HEAD is now at 70d2c39 Merge 1f11e0b into ceb7fb6

i.e. checkout action would merge ref branch on its own.

[janosh]

thanks a lot @DanielYang59! much clearer now, which vars are module-scoped 👍

[janosh]

thanks for uncovering actions/checkout#1359. didn't know about this subtlety. will try to remember if it comes up again

[DanielYang59]

No problem! Thanks for reviewing (always amazed by how much you know about all those APIs)

[janosh]

maybe time for a new release soon? what do you think? anything that should be done first?

[DanielYang59]

Perhaps let me finish #200 perhaps in one or two days? It's largely done, I just want to finish the interactive tester and have a try on it?

But feel free to release a new one now and I'm happy to keep it slow and polish that more :)

[janosh]

sure no hurry, we can definitely wait 2 days or more. looking forward to #200! 👍

[DanielYang59]

I really appreciate that :)