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

use micromamba #123

Merged
merged 6 commits into from
Jul 7, 2024
Merged

Conversation

zacharyburnett
Copy link
Contributor

@zacharyburnett zacharyburnett commented Apr 21, 2023

fixes #197 I think?

@zacharyburnett zacharyburnett marked this pull request as ready for review April 21, 2023 17:37
@Cadair Cadair requested a review from ConorMacBride April 22, 2023 14:02
Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one question about the conda channel. Also, I think the changes to test_tox.yml and tox.ini should be reverted. I added tests with the con_da tox environment is for testing that conda can be configured via an Actions input, and not just through the environment name. (The tests for this project are quite confusing!)

.github/workflows/tox.yml Outdated Show resolved Hide resolved
@zacharyburnett zacharyburnett force-pushed the micromamba branch 3 times, most recently from c6e5193 to a9c8d9b Compare April 27, 2023 19:02
@ConorMacBride
Copy link
Member

I think the assert len(weights) == expected_node_count was an error in pip and was fixed in pip v22.0.3, released over a year ago. Since the pip version is so old, I wonder if python is the system python rather than the one installed by micromamba?

Maybe we should be avoiding pip when using conda? We could install tox and the tox-deps by passing them into create-args instead?

Also, I this silent error looks like we might need to update the conda executable to micromamba in tox.ini.

@zacharyburnett zacharyburnett force-pushed the micromamba branch 3 times, most recently from 63173f0 to 7e112b7 Compare June 9, 2024 17:19
@zacharyburnett
Copy link
Contributor Author

I think the assert len(weights) == expected_node_count was an error in pip and was fixed in pip v22.0.3, released over a year ago. Since the pip version is so old, I wonder if python is the system python rather than the one installed by micromamba?

Maybe we should be avoiding pip when using conda? We could install tox and the tox-deps by passing them into create-args instead?

Also, I this silent error looks like we might need to update the conda executable to micromamba in tox.ini.

@ConorMacBride sorry it took me so long to get back to this! The tests appear to be passing now

@Cadair Cadair requested a review from ConorMacBride June 10, 2024 12:12
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me but I will let @ConorMacBride have the final word.

Thanks @zacharyburnett !

@zacharyburnett
Copy link
Contributor Author

zacharyburnett commented Jun 26, 2024

not to rush anyone; this is ready to merge and fixes the failing test_conda test

Copy link
Member

@ConorMacBride ConorMacBride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zacharyburnett! Sorry for the delay in reviewing

@ConorMacBride ConorMacBride merged commit d6ea421 into OpenAstronomy:main Jul 7, 2024
48 checks passed
@zacharyburnett zacharyburnett deleted the micromamba branch July 8, 2024 12:18
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.

Fix conda support on MacOS
3 participants