Make setup.py consistent with conda recipe

[xylar]

Inconsistencies in setup.py are causing pip check to fail on the conda package.

[xylar]

@tomvothecoder and @forsyth2, it seems like you are testing zstash with a pip based workflow. I would suggest switching to a mamba and boa based workflow where you build the conda package. This will be more similar to how the package is used in E3SM-Unified and elsewhere. The pip installation is missing important tools like mache and mpas_tools, and doesn't check if the conda and pip dependencies are conssitent.

[xylar]

I will do the precomit stuff. I forgot about it.

[xylar]

Okay, all the testing failures have nothing to do with my changes, I think.

[forsyth2]

I would suggest switching to a mamba and boa based workflow where you build the conda package.

Created E3SM-Project/zppy#397 and #252

[forsyth2]

Okay, all the testing failures have nothing to do with my changes, I think.

I ran on Perlmutter -- pre-commit checks pass, unit tests pass. I'm not sure why GitHub is marking failures.

[mahf708]

We can also run the test suite on conda-forge CI if needed

[xylar]

@mahf708, by the time a package gets to conda-forge, it's really too late. Testing needs to be done here.

@forsyth2, my guess is that it's a GitHub issue such as an expired token but I don't know for sure. All I can see is:

Job failed.

[xylar]

@tomvothecoder, here as well, could we get hellp on why CI is failing?

[mahf708]

@mahf708, by the time a package gets to conda-forge, it's really too late. Testing needs to be done here.

Sorry, I didn't meant to replace testing here. Rather, we could run the whole test suite (if reasonable, easy) in the conda-forge build as well to see how they integrate there. These packages are pretty small, so we can definitely add some more tests besides the import and pip check. Looking at the workflow files here, I am not seeing actual testing (or maybe I am missing it?) — is simply pytest . enough?

[xylar]

@mahf708, I see. Yes, we should pytest on conda-forge as well. But the full test suite probably has to be run on an HPC machine because it requires tools for tape archiving that are not going to be available anywhere else.

[xylar]

@mahf708, it's not pytest it's unittest:
https://github.com/E3SM-Project/zstash/blob/main/.github/workflows/build_workflow.yml#L50

python -m unittest tests/test_*.py

Feel free to add that to conda-forge/staged-recipes#21883. I think unittest is built in but maybe a dependency is needed?

Also maybe look at the zppy workflow to see what the corresponding tests would be. Maybe identical.

[tomvothecoder]

@tomvothecoder and @forsyth2, it seems like you are testing zstash with a pip based workflow. I would suggest switching to a mamba and boa based workflow where you build the conda package. This will be more similar to how the package is used in E3SM-Unified and elsewhere. The pip installation is missing important tools like mache and mpas_tools, and doesn't check if the conda and pip dependencies are conssitent.

Good point on pip install missing dependencies. @forsyth2 can you open an issue and look into this?
You would need to do some modifications to the workflows to build and install zstash using mamba instead of pip install.

@tomvothecoder, here as well, could we get hellp on why CI is failing?

I think GitHub Actions was down at the time you kicked off the workflows. I reran the workflows and they now pass.

[forsyth2]

can you open an issue and look into this?
You would need to do some modifications to the workflows to build and install zstash using mamba instead of pip install.

That's #252

[xylar]

There seem to be questions about which of these is correct. @forsyth2 and @lukaszlacinski, can you both weight in?

[forsyth2]

I'm really not sure.

The changes to these values come from https://github.com/E3SM-Project/zstash/pull/201/files, which itself was from @lukaszlacinski's commit lukaszlacinski@13c2660.

As @tomvothecoder mentioned in #201 (comment): "The workaround was to constrain globus-sdk >=2, <3 in setup.py in #188")

[xylar]

@forsyth2, it looks to me like setup.py is correct, then and that @lukaszlacinski didn't know that he needed to keep conda/meta.yaml consistent. But that also means that these constraints aren't in E3SM-Unified so hopefully we're still okay...

[xylar]

@forsyth2, I have updated this PR to what I hope is the best of both worlds. I think it's likely that @lukaszlacinski over-constrained six and fair-research-login in lukaszlacinski@13c2660. But I think the stricter constraints on globus-sdk make sense.

Obviously, it would be great to have you weigh in, @lukaszlacinski.

[xylar]

I have also updated GitHub actions and pre-commit to be consistent with E3SM-Project/zppy#395

[forsyth2]

I was able to build the environment with this branch, but when I ran the unit tests, I got:

======================================================================
ERROR: testLs (tests.test_globus.TestGlobus)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/global/u1/f/forsyth/zstash/tests/test_globus.py", line 183, in testLs
    self.helperLsGlobus(
  File "/global/u1/f/forsyth/zstash/tests/test_globus.py", line 161, in helperLsGlobus
    self.preactivate_globus()
  File "/global/u1/f/forsyth/zstash/tests/test_globus.py", line 62, in preactivate_globus
    native_client.login(no_local_server=True, refresh_tokens=True)
  File "/global/homes/f/forsyth/miniconda3/envs/zstash_n250/lib/python3.9/site-packages/fair_research_login/client.py", line 157, in login
    auth_code = self.get_code(requested_scopes, refresh_tokens,
  File "/global/homes/f/forsyth/miniconda3/envs/zstash_n250/lib/python3.9/site-packages/fair_research_login/client.py", line 197, in get_code
    auth_url = self.client.oauth2_get_authorize_url(
TypeError: oauth2_get_authorize_url() got an unexpected keyword argument 'additional_params'

----------------------------------------------------------------------
Ran 64 tests in 271.141s

FAILED (errors=1)

@lukaszlacinski This leads me to believe there is an issue in the version constraints.

[forsyth2]

Looking at #201, it was lukaszlacinski@13c2660 that fixed that exact error.

[xylar]

@forsyth2, okay, it seems like the version of fair-research-login is most likely to be responsible since the constraints on globus are the same as what @lukaszlacinski had. Let's try going back in that case.

[xylar]

@forsyth2, could you give it another try? Could you also make an issue about seeing if we can support the latest fair-research-login? Or if the problem isn't with zppy and instead with globus, we should post an issue on their GitHub (or see if there already is one) about fair-research-login being underconstrained.

[forsyth2]

could you give it another try?

Tests pass now! Should I go ahead and merge it?

Could you also make an issue about seeing if we can support the latest fair-research-login?

#253

[xylar]

Yes, please merge!