Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workflow #73

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Workflow #73

wants to merge 2 commits into from

Conversation

voetberg
Copy link
Contributor

Adding a test-on-push action.

@bnord
Copy link
Contributor

bnord commented May 31, 2023

Should I cite the check failure as an issue, or should I try to do something about it?

Failed to install /home/runner/.cache/pypoetry/artifacts/23/d4/ff/1c52c84e721934075987a4054f149ffe1c71a41055a1bde7f7daa6fee6/pymaster-1.5.tar.gz

at /opt/hostedtoolcache/Python/3.10.11/x64/lib/python3.10/site-packages/poetry/utils/pip.py:58 in pip_install
54│
55│ try:
56│ return environment.run_pip(*args)
57│ except EnvCommandError as e:
→ 58│ raise PoetryException(f"Failed to install {path.as_posix()}") from e
59│

Error: Process completed with exit code 1.

@voetberg
Copy link
Contributor Author

This is included in issues related to packaging. Sam has the details I believe. Not sure if they're in a different issue already.

@voetberg
Copy link
Contributor Author

For reference - this error is due to a problem with pymaster struggling with anything that isn't conda due to some Incredibly outdated C package integration.

Actual errorstack comes from:

         File "/tmp/pip-build-env-vfaz83uc/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 341, in get_requires_for_build_wheel
            return self._get_build_requires(config_settings, requirements=['wheel'])
          File "/tmp/pip-build-env-vfaz83uc/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 323, in _get_build_requires
            self.run_setup()
          File "/tmp/pip-build-env-vfaz83uc/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 487, in run_setup
            super(_BuildMetaLegacyBackend,
          File "/tmp/pip-build-env-vfaz83uc/overlay/lib/python3.10/site-packages/setuptools/build_meta.py", line 338, in run_setup
            exec(code, locals())
          File "<string>", line 12, in <module>
        ModuleNotFoundError: No module named 'numpy'
        [end of output]

@humnaawan
Copy link
Collaborator

@voetberg sorry can you please say why you think the issue is something to do with C and not the missing numpy module?

in my experience, conda install of pymaster works without errors - can we use that (with even the fix for the new Macs which should be doable after the pip install of this package) instead of the pip install of pymaster?

@cavestruz
Copy link
Contributor

Note, if Continuous Integration ends up taking more than this week to include, I think it is sufficient to add in the README instructions for a new user/potential developer to simply run pytests and the example notebook to make sure everything is working.

@voetberg
Copy link
Contributor Author

voetberg commented Jun 7, 2023

@humnaawan

So numpy is installed, (in the action, numpy is the 2nd thing installed I believe). It counts numpy as missing during the pymaster install because it's running some legacy setup things that use a legacy version of numpy that cannot build (because of numpy's c backend components, to my understanding).

I was working with Sam on this, and his explanation of the issue requires that you find a specific distribution of pymaster that requires you spoof a conda env, which you can't do if you already have an environment set up. Which is silly.

@cavestruz

I do wholly disagree honestly; having the code in a state that can be run without error from a fresh install is critical to a codebase's adoption and future usability. If a future package tries to use this code it just pushes this issue forward and makes it harder to install for other codebases.

@cavestruz
Copy link
Contributor

@voetberg - I think I'm then a bit confused on which problems are affecting what.

From the chat with @samueldmcdermott earlier today, I understood that

  • all tests pass from a manual pytest execution,
  • the issue with pip installation occurs only with the CI set-up(?)

For the latter point, is it insufficient to just keep to an overall conda install, such that namaster works as @humnaawan suggested?

I completely agree that the code should be runnable without error from a fresh install of whatever the latest stable version release might be. But, I think what were working with is such that it needs to be a conda install, not a pip install for that to be the case. Does "runnable without error from a fresh install" necessitate CI?

If I'm understanding the underlying problem, it's either
(a) work with namaster installations with the friendly path forward (conda install) and sacrifice CI, OR,
(b) finagle something with namaster installations that could be quite challenging in order to enable CI.

Please let me know if I'm missing something, as I can imagine a bit of broken telephone happening in two separate sets of conversations.

@cavestruz
Copy link
Contributor

Btw, I definitely would like to see this repo have CI. But, I wouldn't want that to be what holds up a JOSS submission for a v1.0 of the code release.

If namaster isn't nicely pip installable because of weird numpy version dependencies, then that's something on the namaster developers' plate. (If their repo doesn't yet have an issue for this, can someone here create one?)

If there's a relatively fast/straightforward fix in the next 2 days, like pinning a numpy version in our codebase's installation steps, I'd support that effort as well. But, I tend to get nervous if things start to look like a version rabbit hole.

My primary suggestion is to go along with the suggestion of @humnaawan to change installation instructions in the README to be a conda install, make sure everything passes with pytest (and that everything in the example notebook runs), and so the process is reproducible for new users. When namaster developers get the pip installation stable for namaster, we can then propagate that to this code base. But, that may happen after the desired timeline for a JOSS submission.

@voetberg
Copy link
Contributor Author

voetberg commented Jun 8, 2023

@cavestruz
Your understanding seems to be my understanding as well, so probably minimal telephone problems haha.

I do agree a v1.0 for JOSS it's totally fine not to have CI or clean problem-less pip install, I guess I'm thinking more long term than this initial release. I stamped on the gas on the assumption you were fine leaving things as is permanently, apologies if that came off as aggressive or anything. So solution a there is almost certainly fine for a v1.0.

We can also get around the CI problems by changing the test/publish actions to include the fix @humnaawan is recommending, but the user probably won't have that on their radar (as far as when they initially try to install the package). But again, we can probably push this to after the joss submission.

I'll also see if I can get some screenshots of the install problems and a proposed solution and raise that in the pymaster repo if it isn't already being worked on.

@voetberg
Copy link
Contributor Author

voetberg commented Jun 8, 2023

Looks like they are minimally aware of the issue, but don't seem to intend to do anything about it (re: This pr with the same issue that's been open since 2021....... LSSTDESC/NaMaster#143 )

@cavestruz
Copy link
Contributor

Looks like they are minimally aware of the issue, but don't seem to intend to do anything about it (re: This pr with the same issue that's been open since 2021....... LSSTDESC/NaMaster#143 )

Ah, okay. Hmmm, I guess they'll see that the PR is relevant to our repo once we publish this code if we link a new issue to this. Would it be sufficient to have a new issue with some tag like V2.0 release titled "CI and pip install-ablity" with their Issue 143 linked?

I stamped on the gas on the assumption you were fine leaving things as is permanently

Not at all - I was worried I came across as bulldozing time you'd already spent hunting this down, so wanted to clarify that I do want to see the current understanding laid out somewhere and that I value potential paths to a solution implemented in this code base's near lifespan, just not necessarily as a pre-requisite for the JOSS submission (I should've clarified that in the first message I posted).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants