Adapter and Docker #67

fmigneault · 2019-05-13T23:22:22Z

This PR adds:

lightweight docker image building (requirement for Ouranosinc/Magpie#182)
AdapterInterface class that allows overriding 'store' implementations.
DefaultAdapter is used when twitcher.adapter = default or is missing, which calls the store creation with the original methodology.
multiple makefile/bumpversion/setup improvements accumulated over time

… time

cehbrecht

I have only slightly tested it but code looks fine :)

I'm struggling with the Makefile. Please have a look and see my comments.

cehbrecht · 2019-05-14T11:26:26Z

Makefile

-CONDA_ENV := $(APP_NAME)
+# Conda
+CHECK      ?= shell command -v
+CONDA_HOME ?= $($(CHECK) conda info --base 2> /dev/null)


setting CONDA_HOME didn't work: use previous cmd $(shell conda info --base 2> /dev/null)

This doesn't work for me.

tmp: @echo "THE RESULT: $(shell conda info --base 2> /dev/null)"

make tmp >> THE RESULT:

which conda >> conda () { <...> }

** this is the miniconda output on ubuntu

With only conda info --base 2> /dev/null (without shell), I get the actual conda home path.

cehbrecht · 2019-05-14T11:27:54Z

Makefile

+CONDA_ENV  ?= $(APP_NAME)
+CONDA_BIN  := $(CONDA_HOME)/bin/conda
+CONDA_TARGET_PREFIX := readlink -e "$(CONDA_HOME)/envs/$(CONDA_ENV)" 2> /dev/null
+CONDA_ACTUAL_PREFIX := readlink -e "${CONDA_PREFIX}" 2> /dev/null


CONDA_PREFIX is not always defined. On my mac I'm using anaconda system installation and I have to run conda activate to make conda available.

The idea here is that, CONDA_PREFIX is indeed not defined, therefore the conda environment is not activated, so the script activates it with CONDA_CMD before doing any other operation (pip install, etc.).
If the "right" CONDA_ENV == twitcher conda environment is already activated because the user already did conda activate twitcher, CONDA_PREFIX points to it and there is no need for CONDA_CMD to be executed.

cehbrecht · 2019-05-14T11:29:31Z

Makefile

+CONDA_ACTUAL_PREFIX := readlink -e "${CONDA_PREFIX}" 2> /dev/null
+# don't activate and update conda env if it already is activated
+# (avoids long conda 'solving environment' time)
+ifeq ($(shell $(CONDA_TARGET_PREFIX)),$(shell $(CONDA_ACTUAL_PREFIX)))


This check is "true" when there is both no conda env and no CONDA_PREFIX ... not what we want.

On my side, this works as intended.
On the other hand, the previously used commands were failing on my side :

CONDA := $(shell command -v conda 2> /dev/null) ANACONDA_HOME := $(shell conda info --base 2> /dev/null)

What values are you getting (use make debug) ?
I would like to investigate to find a method that works for everyone.
I believe that conda variants can be very intricate depending on which one it is (anaconda, miniconda, etc.)

On ubuntu docker image I get:

$ make debug Following variables are used: SHELL: /bin/sh CHECK: shell command -v APP_ROOT: /opt/twitcher APP_NAME: twitcher BUMP_XARGS: --verbose --allow-dirty CONDA_HOME: CONDA_BIN: /bin/conda CONDA_ENV: twitcher CONDA_MSG: Conda environment already activated CONDA_CMD: echo Conda environment already activated; CONDA_TARGET_PREFIX [literal]: readlink -e /envs/twitcher 2> /dev/null CONDA_ACTUAL_PREFIX [called]: CONDA_TARGET_PREFIX [literal]: readlink -e 2> /dev/null CONDA_ACTUAL_PREFIX [called]: DOCKER_TAG: birdhouse/twitcher:0.4.0

miniconda is installed:

which conda /opt/anaconda/bin/conda

If you run make CHECK="command -v" debug, does your CONDA_HOME display a valid path instead of blank?

I suspect that the $(shell $(...)) syntax might be causing the issue.
If that's the case, ifeq (`$(CONDA_TARGET_PREFIX)`,`$(CONDA_ACTUAL_PREFIX)`) instead of the highlighted code will probably do the trick.

I get the same output for both on my side.

I run this on my ubuntu docker container ... but does not change it:

$ make CHECK="command -v" debug Following variables are used: SHELL: /bin/sh CHECK: command -v APP_ROOT: /opt/twitcher APP_NAME: twitcher BUMP_XARGS: --verbose --allow-dirty CONDA_HOME: CONDA_BIN: /bin/conda CONDA_ENV: twitcher CONDA_MSG: Conda environment already activated CONDA_CMD: echo Conda environment already activated; CONDA_TARGET_PREFIX [literal]: readlink -e /envs/twitcher 2> /dev/null CONDA_ACTUAL_PREFIX [called]: CONDA_TARGET_PREFIX [literal]: readlink -e 2> /dev/null CONDA_ACTUAL_PREFIX [called]: DOCKER_TAG: birdhouse/twitcher:0.4.0

Both on ubuntu and on macos I'm using bash. Do you have a different shell which would cause a difference?

I think it is because of miniconda alternative configs that define conda.csh scripts to do some pre-processing steps before activating the environment. This causes the conda "bin" to actually point to another script which messes up commands like which.
If have found that CONDA_EXE points to the real conda bin path though.
So if it does on your side too in both situations when env is (de)activated, I could adapt the script to use this variable.

I have CONDA_EXE only when I run conda activate:

$ conda activate $ env | grep -i conda $ CONDA_EXE=/usr/local/anaconda3/bin/conda

But the env is not defined when I just put the conda-bin in PATH.

Does which help?
https://github.com/bird-house/pyramid-phoenix/blob/5c5a4f73778c31abdee13ce004dd651b43f24247/docs/Makefile#L11

Makefile

cehbrecht · 2019-05-14T11:39:34Z

Why is the adapter necessary? Do you already have a different adapter implementation?

There is an AdapaterInterface. Are the adapter implementations supposed to implement the
twitcher.store.ServiceStoreInterface etc?

fmigneault · 2019-05-14T13:31:50Z

@cehbrecht
Yes, that's exactly it. Magpie has an implementation of AdapaterInterface here (although maybe needs updating because of Python 3 and recent package changes).
The store interfaces provide abstract function definitions that have to be specified by the adapter that wishes overriding them.

fmigneault · 2019-05-14T14:07:38Z

@cehbrecht
I have temporarily defined in the makefile DOCKER_TAG := birdhouse/twitcher to do docker image build/push.
Will you have such repo available on dockerhub?

Required for Ouranosinc/Magpie#182 to pull 'base' image of MagpieAdapter+Twitcher.

cehbrecht · 2019-05-14T15:11:36Z

@cehbrecht
I have temporarily defined in the makefile DOCKER_TAG := birdhouse/twitcher to do docker image build/push.
Will you have such repo available on dockerhub?

Yes, we can use this docker tag:
https://hub.docker.com/u/birdhouse/

fmigneault · 2019-05-15T16:37:15Z

@cehbrecht
I have reverted the COMDA_HOME with $(shell conda info --base 2> /dev/null) in 04b795c
I adjusted something in my shell to make it work.
Would like your confirmation that it is doing what it should for you too. (thanks again for your support btw)

cehbrecht · 2019-05-16T07:59:08Z

make debug looks good:

make debug
Following variables are used:
  SHELL:                         /bin/sh
  APP_ROOT:                      /opt/twitcher
  APP_NAME:                      twitcher
  BUMP_XARGS:                    --verbose --allow-dirty
  CONDA_HOME:                    /opt/anaconda
  CONDA_BIN:                     /opt/anaconda/bin/conda
  CONDA_ENV:                     twitcher
  CONDA_MSG:                     Will activate conda environment with 'CONDA_CMD'
  CONDA_CMD:                     echo Activating conda env 'twitcher' ...; source /opt/anaconda/bin/activate twitcher;
  CONDA_TARGET_PREFIX [literal]: readlink -e /opt/anaconda/envs/twitcher 2> /dev/null
  CONDA_ACTUAL_PREFIX [called]:  /opt/anaconda/envs/twitcher
  CONDA_TARGET_PREFIX [literal]: readlink -e  2> /dev/null
  CONDA_ACTUAL_PREFIX [called]:  
  DOCKER_TAG:                    birdhouse/twitcher:0.4.0

But still there is an issue ... make install does not install in the conda enviroment.

The manual installation still works:

$ conda env create
$ source activate twitcher
$ pip install -e .
$ pip install -r requirements_dev.txt
$ make test

I would merge this PR and open a new issue for the Makefile. Makefile is "only" needed for user-friendly installation.

cehbrecht

Code looks good. Makefile issue will be solved in a new PR.

cehbrecht · 2019-05-16T08:03:10Z

@fmigneault merged. Thanks :)

fmigneault added 6 commits May 13, 2019 14:35

transfer basic adapter setup + dockerfile

daf0e2e

add more makefile targets and improve conda activation/update solving…

766661a

… time

update adapter to use request required for dbsession

2f864ae

update docker build and related configs/targets/requirements

585d324

add adapter use where missing

27713ad

fix pep8

936511e

cehbrecht self-requested a review May 14, 2019 08:18

cehbrecht added the enhancement label May 14, 2019

cehbrecht added this to the 0.5.0 milestone May 14, 2019

cehbrecht requested changes May 14, 2019

View reviewed changes

fmigneault added 6 commits May 14, 2019 13:43

revert bootstrap + use extra requires for dev install

7acdb1d

twitcher frontpage message

3bf3504

fix adapter mismatch input

18ed911

add checks to all targets using conda

0f2e6c4

fix typo in config

59b7fa1

revert CONDA_HOME with shell command

04b795c

fix adapter message

c1186f7

cehbrecht self-requested a review May 16, 2019 07:59

cehbrecht approved these changes May 16, 2019

View reviewed changes

cehbrecht merged commit ad623c0 into bird-house:master May 16, 2019

cehbrecht mentioned this pull request May 16, 2019

Makefile has issues with conda in different enviroments. #68

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapter and Docker #67

Adapter and Docker #67

fmigneault commented May 13, 2019

cehbrecht left a comment

cehbrecht May 14, 2019 •

edited

Loading

fmigneault May 14, 2019

cehbrecht May 14, 2019

fmigneault May 14, 2019 •

edited

Loading

cehbrecht May 14, 2019

fmigneault May 14, 2019

cehbrecht May 14, 2019

cehbrecht May 14, 2019

fmigneault May 14, 2019 •

edited

Loading

cehbrecht May 15, 2019

cehbrecht May 15, 2019

fmigneault May 15, 2019

cehbrecht May 15, 2019

cehbrecht commented May 14, 2019

fmigneault commented May 14, 2019

fmigneault commented May 14, 2019

cehbrecht commented May 14, 2019

fmigneault commented May 15, 2019

cehbrecht commented May 16, 2019

cehbrecht left a comment

cehbrecht commented May 16, 2019

Adapter and Docker #67

Adapter and Docker #67

Conversation

fmigneault commented May 13, 2019

cehbrecht left a comment

Choose a reason for hiding this comment

cehbrecht May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmigneault May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmigneault May 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cehbrecht commented May 14, 2019

fmigneault commented May 14, 2019

fmigneault commented May 14, 2019

cehbrecht commented May 14, 2019

fmigneault commented May 15, 2019

cehbrecht commented May 16, 2019

cehbrecht left a comment

Choose a reason for hiding this comment

cehbrecht commented May 16, 2019

cehbrecht May 14, 2019 •

edited

Loading

fmigneault May 14, 2019 •

edited

Loading

fmigneault May 14, 2019 •

edited

Loading