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

Improve usage of setup-miniconda #249

Merged
merged 3 commits into from
Jan 13, 2023
Merged

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Jan 12, 2023

Description of proposed changes

See commit messages.

Related issue(s)

Adapted from nextstrain/augur#1126.

Testing

  • Checks pass

Based on the input description in action.yml¹, the default value is
"flexible", and "true" is just an alias to "flexible".

¹ https://github.com/conda-incubator/setup-miniconda/blob/7e642bb2e4ca56ff706818a0febf72bb226d348d/action.yml#L173-L186
@victorlin victorlin requested a review from a team January 12, 2023 23:51
@victorlin victorlin self-assigned this Jan 12, 2023
@victorlin victorlin force-pushed the victorlin/update-setup-miniconda branch from d767f60 to d6cf607 Compare January 13, 2023 00:04
@victorlin
Copy link
Member Author

Hmm, there are a few failing jobs:

The first 3 all fail on this step:

- name: Check python version
shell: bash -l -eo pipefail {0}
run: |
# Assert that we're on the expected Python version, in case the GH
# Actions environment is messed up.
type python
python --version
type python3
python3 --version
python --version | grep -F 'Python ${{ inputs.python-version }}.'
python3 --version | grep -F 'Python ${{ inputs.python-version }}.'
[[ "$(python --version)" == "$(python3 --version)" ]]

I don't understand why they are failing, since the outputs of python --version and python3 --version both seem to be Python 3.6.13 :: Anaconda, Inc.. I'd imagine the error should come from the check [[ "$(python --version)" == "$(python3 --version)" ]], but the output of the previous grep command is missing from the logs, so maybe it has to do with that?

The failure on the 4th job is a known issue with the libmamba + Windows, though test-standalone (os=windows-2022, target=x86_64-pc-windows-msvc) was successful.

@victorlin
Copy link
Member Author

I looked into the Python 3.6 failures a bit more by splitting the step up to see where it's failing: victorlin@7171b90

Running CI on that commit makes it apparent that exit code 1 is coming from python --version | grep -F 'Python 3.6.'. Which is weird because there is a match! Gonna debug some more to figure out what's going on here.

@victorlin
Copy link
Member Author

Mystery solved! The output of python --version here is Python 3.6.13 :: Anaconda, Inc. which differs from that of the last passing CI run, Python 3.6.15. Oddly, this Anaconda version of Python sends the output of python --version to stderr, even though that is old behavior on official Python versions as of 3.4.

Plus, only the 3.6 version of Python from the default anaconda channel is marked with Anaconda, Inc. and has this odd output behavior (maybe due to the end-of-life status of 3.6). A small script to summarize the behavior:

wget https://anaconda.org/anaconda/python/3.6.13/download/linux-64/python-3.6.13-h12debd9_1.tar.bz2
tar -xf python-3.6.13-h12debd9_1.tar.bz2 --one-top-level
./python-3.6.13-h12debd9_1/bin/python --version 2> stderr 1> stdout
cat stderr
# Python 3.6.13 :: Anaconda, Inc.
cat stdout

wget https://anaconda.org/anaconda/python/3.7.15/download/linux-64/python-3.7.15-h7a1cb2a_1.tar.bz2
tar -xf python-3.7.15-h7a1cb2a_1.tar.bz2 --one-top-level
./python-3.7.15-h7a1cb2a_1/bin/python --version 2> stderr 1> stdout
cat stderr
cat stdout
# Python 3.7.15

Currently CI passes outside of this PR because it is pulling Python from conda-forge, not anaconda. I plan to make a change to explicitly pull Python from conda-forge to avoid the weirdness here. Maybe that should be done in other repos too.

victorlin added a commit that referenced this pull request Jan 13, 2023
Python from the default anaconda channel can be unreliable¹. Use
conda-forge which worked previously.

¹ #249 (comment)
@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

by splitting the step up to see where it's failing

Nod. It'd probably be useful to add -x to the shell: command line for the combined step, so we can see what's failing that way.

(Also, debugging that sort of failure is right up the alley of our new "debugging runner" workflow. 🙃)

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

Oddly, this Anaconda version of Python sends the output of python --version to stderr…
[…]
I plan to make a change to explicitly pull Python from conda-forge to avoid the weirdness here.

Good sleuthing. Agreed we probably want the conda-forge build instead of the Anaconda build.

In addition, I guess we could also change:

python --version | grep -F 'Python 3.6.'

to:

python --version |& grep -F 'Python 3.6.'

to grep both stdout and stderr. (Or maybe only grepping stdout is a feature, e.g. it caught this unintentional change.)

Although |& isn't supported on the macOS system version of bash—which we're probably using here?—so we'd actually have to use 2>&1 | instead.

Based on the input description in action.yml¹, the default value is
already "test" which means it should be auto-activated.

¹ https://github.com/conda-incubator/setup-miniconda/blob/7e642bb2e4ca56ff706818a0febf72bb226d348d/action.yml#L79-L91
@victorlin victorlin force-pushed the victorlin/update-setup-miniconda branch from 06150f7 to 5239f4b Compare January 13, 2023 21:15
@victorlin
Copy link
Member Author

After comparing the workarounds in conda-incubator/setup-miniconda#274, I replaced e1036f6...06150f7 with 5239f4b which is a simpler way of accomplishing the same thing.

@victorlin
Copy link
Member Author

(Also, debugging that sort of failure is right up the alley of our new "debugging runner" workflow. 🙃)

Oh, I'll keep that in mind for next time!

Or maybe only grepping stdout is a feature, e.g. it caught this unintentional change.

Yeah, it was good to have here. I'll plan to leave it as-is.

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

which is a simpler way of accomplishing the same thing.

Ahh, I like this a lot more. Less change, less weirdness around setting an alternative solver, and avoids the Windows DLL issue.

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

Hmm, but looks like conda-incubator/setup-miniconda@v2 is still getting packages from the Anaconda channel, not conda-forge, when creating the test env. (Which we ultimately see as job failure because of the stdout/stderr issue again.) The base env is created only from conda-forge, though.

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

Looked into that a little more, and it seems like the channel option to this action is not working as expected. It adds the channel to the existing config, but one of those configs already has the defaults channel (and one of them only has conda-forge).

For example:

  ==> C:\Miniconda3\.condarc <==
  channels:
    - conda-forge
  
  ==> C:\Users\runneradmin\.condarc <==
  auto_update_conda: False
  auto_activate_base: True
  notify_outdated_conda: False
  changeps1: False
  pkgs_dirs:
    - C:\Users\runneradmin\conda_pkgs_dir
  channels:
    - bioconda
    - defaults
  always_yes: True

which subsequent conda config --show output in the log shows combined as:

  channels:
    - bioconda
    - defaults
    - conda-forge

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

(I'm particularly interested in this issue because I think it will resolve the CI failures in my recently-opened #250. :-)

The option to use mamba-version in this action is still marked as
experimental and has shown to be problematic with transient failures.

An alternative is to use Mambaforge, which is a Miniforge variant that
comes with Mamba in the base environment. It uses the conda-forge
channel as the only default channel¹.

However, conda-forge must be added to the channels input to the
setup-miniconda action, since it creates and uses a separate condarc
file which contains the specified channels along with defaults², and
defaults should be avoided at least for the Python version³.

¹ https://github.com/conda-forge/miniforge/blob/871e98647a95b99a769dd9f40669d06c44514bca/README.md
² #249 (comment)
³ #249 (comment)
@victorlin victorlin force-pushed the victorlin/update-setup-miniconda branch from 5239f4b to 4a76497 Compare January 13, 2023 22:00
@victorlin
Copy link
Member Author

@tsibley yeah this is odd. Trying 4a76497 which includes conda-forge in the list of channels. Doesn't remove defaults, but hopefully it gets us past CI for now.

@victorlin
Copy link
Member Author

the channel option to this action is not working as expected

Looks like there's an open issue for this: conda-incubator/setup-miniconda#207

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

🙌

Doesn't remove defaults, but hopefully it gets us past CI for now.

We could also set channel-priority: strict in the action's inputs to essentially ignore defaults nearly all the time.

@victorlin victorlin merged commit c600d2d into master Jan 13, 2023
@victorlin victorlin deleted the victorlin/update-setup-miniconda branch January 13, 2023 22:46
@victorlin
Copy link
Member Author

There's lots of ways to reliably get away from defaults:

  • nodefaults
  • --override-channels
  • conda config --remove channels defaults

I'll think about which one to use and make another PR.

@tsibley
Copy link
Member

tsibley commented Jan 13, 2023

Also ok by me to leave things as-is for now. defaults doesn't seem to be causing any issues here (…yet?).

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

Successfully merging this pull request may close these issues.

None yet

2 participants