Closed jonwzheng closed 5 months ago
So this simple change actually dramatically changed the conda environment, see the diff here: https://www.diffchecker.com/RJ4dcsLN/
@jonwzheng I suspect that the issue is clang
- the new version of muq2
results in clang
being installed and used for compiling RMG, whereas we advise using gcc
. Going to tack on a commit to try and deal with this.
My trick worked, so the Mac build works again, if we do want to stick with this, we should add the specification of the environment variables to the docs.
I'm getting ahead of myself here, but muq
only has conda binaries supporting up to Python 3.10 - when we want to make the transition to 3.11+ (#2635) this would be a problem.
We use muq
in one place (the global uncertainty module) and we list it as 'optional' in the docs. I'm actually tempted to say that we should actually remove it from the environment file and instead add installation instructions to the docs for those who want it.
Long term we should really think about the future of muq
- the conda-forge
team considers the conda recipe "comatose": https://github.com/conda-forge/conda-forge-pinning-feedstock/issues/5231#issue-2022972006
Summarizing some offline discussion:
muq
to see if they are willing to take up the conda binary building again and get versions out that supporting python 3.11+muq
themselvesRegardless, @jonwzheng can you add a line in the docs about specifying CC
and CXX
to avoid Mac using a compiler other than gcc
?
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Just sent an email poking the muq2
team. Will work on updating the docs shortly.
Instead of requiring users to specify compiler, do you know if we could just modify the Makefile to use those compilers by default when make
is called?
Good point, I think we could just put CC=gcc
in the top of the Makefile
actually
okay i'll give it a try and push that commit to this branch.
Nice, look like this worked!
I think the only outstanding thing to do for this PR is to get another review on it. Could you request someone @jonwzheng?
When we do the Python upgrade we will also target Python 3.10 so that the muq
binaries can be installed, or else update the build instructions to tell people to install muq
some other way.
sounds good, thanks! NGL, I wasn't confident that change would work on the first pass through...
After all of the changes we've made that should've worked on the first try, I am willing to accept this nice little treat from the universe.
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
@jonwzheng this PR as well as #2680 and #2681 all basically do the same thing - do you want me to just merge those two into this one, and then this one into main
to avoid a bunch of extra rebasing. All three of them seem to be working, BTW
sure, that sounds good. Let's make sure the comments in #2680 get preserved as it's pretty useful context.
Update: The comment in question:
2596 is also working on implementing linear solving with scipy, which combined with #2623 would remove our need of lpsolve55
the former is blocked by upgrading the Python version (#2635) which will be made easier by merging this pr thus, we merge this PR as a stopgap to (possibly) eventually removing lpsolve completely
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
@rwest could you take a look at this? We just changed some packages to the conda-forge
version, and one minor change to the Makefile to ensure the correct C compiler is used. More thorough summary at the top of the PR, if needed. Thanks!
@rwest thanks for the quick review and bug catch! I've cleaned up the commit history and fixed the line.
WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
This PR changes a number of
rmg
channel packages to their officialconda-forge
variants maintained by other people:rmg::muq2
->conda-forge::muq
(different name, same version)rmg::lpsolve55
->conda-forge::lpsolve55
rmg::py_rdl
->conda-forge::ringdecomposerlib-python
(different name, same package and version)The first change also required adding a new line to the
Makefile
to ensure that correct compiler is used when installing RMG.