search

diffpy / diffpy.pdffit2

real space structure refinement to atomic PDF
Other
4 stars 18 forks source link

Workflow edits v2 #29

Closed 8bitsam closed 3 months ago

8bitsam commented 3 months ago

Trying this again, updated the actions and using conda-incubator instead now.

8bitsam commented 3 months ago

It seems that the Upload Artifacts step is failing due to permissions issues, maybe this can be fixed by refreshing user access tokens.

The macos-latest failing is likely caused by running on ARM, we should decide if continuing to troubleshoot is worth it or reverting to testing on x86 is better (use macos-13 instead for this).

sbillinge commented 3 months ago

It seems that the Upload Artifacts step is failing due to permissions issues, maybe this can be fixed by refreshing user access tokens.

The macos-latest failing is likely caused by running on ARM, we should decide if continuing to troubleshoot is worth it or reverting to testing on x86 is better (use macos-13 instead for this).

It looks as if there is progress there, but why is it trying to upload any artifacts? Shouldn't it just be running tests on a PR?

sbillinge commented 3 months ago

I had a look at the workflow and we don't want to use it to do a release so we can just release the upload artifacts action and end at the run-tests. Let's also not do the matrix for now. Just run it on ubuntu, which will be sufficient for the cookiecuttering.

We want to do the full matrix before we do the release, and would like to have it also test on the arm chips if we can. If there are issues wity PycifRw we can maybe see if there is a better maintained package for parsing cif files?

8bitsam commented 3 months ago

I removed the artifacts and the tests now pass on ubuntu + windows. I'll switch to having it just test on ubuntu for the time being, at least until after the cookiecutter stuff is done.

8bitsam commented 3 months ago

We want to do the full matrix before we do the release, and would like to have it also test on the arm chips if we can. If there are issues wity PycifRw we can maybe see if there is a better maintained package for parsing cif files?

I looked into this and it doesn't seem like there's any real alternatives to PycifRW. I'm still not sure if it has to do with the setup.py or is just an issue with PycifRW itself, but like you said using ubuntu only should be fine for now.

8bitsam commented 3 months ago

It seems that pre-commit isn't passing, but that might be easier to fix after cookiecutting. I think this should be good to merge now, if all looks fine?

sbillinge commented 3 months ago

@8bitsam please can you move over the .flake8 config file and the [tool.black] config in pyproject.toml (if there is no toml file just make one that only contains [tool.black] for now) so we can see where we are with these things with the correct config. Also, make sure to run pre-commit install at the command prompt in all your repos (but especially this one) if you haven't already. Then hopefully we won't be relying on the ci to run black.

The PR basically looks good to merge but we should be able to pass a few more things in pre-commit.ci with these changes above and it will give a better idea about what is left to be done.

sbillinge commented 3 months ago

@8bitsam also, please make sure to make an issue to turn back on the things we turned off.....

8bitsam commented 3 months ago

@8bitsam please can you move over the .flake8 config file and the [tool.black] config in pyproject.toml (if there is no toml file just make one that only contains [tool.black] for now) so we can see where we are with these things with the correct config. Also, make sure to run pre-commit install at the command prompt in all your repos (but especially this one) if you haven't already. Then hopefully we won't be relying on the ci to run black.

The PR basically looks good to merge but we should be able to pass a few more things in pre-commit.ci with these changes above and it will give a better idea about what is left to be done.

I've commented out some stuff on pre-commit-config.yaml to get ci to pass for now. This can be patched up on a separate PR since this PR is sort of step 0 to the "cookiecutter workflow" written on the repo, it's just getting tests passed. I made an issue for this (see #30).