Update pyenv pyenvinstall Make targets #2517

dalf · 2021-01-29T07:19:36Z

What does this PR do?

Change about utils/makefile.python:

make pyenv makes sure that there is a python executable in the virtualenv, if not the virtualenv is installed again.
make pyenvinstall makes to install pyyaml before running setup.py (setup.py requires pyyml to read searx/settings.yml)

Why is this change important?

github action workflow caches the ./local directory :

Lines 25 to 32 in 71d6697

    
           - name: Cache Python dependencies 
        
             id: cache-python 
        
             uses: actions/cache@v2 
        
             with: 
        
               path: ./local 
        
               key: python-${{ matrix.os }}-${{ matrix.python-version }}-${{ hashFiles('requirements*.txt', 'setup.py') }} 
        
           - name: Install Python dependencies 
        
             if: steps.cache-python.outputs.cache-hit != 'true'

sometimes this cache can be corrupted (it is not clear how the cache gets corrupted).
this PR ensure that make test works as expected even if
- the python interpreter is missing
- the virtualenv exists but pyyaml is missing

How to test this PR locally?

make pyenvinstall
rm ./local/py3/bin/python
make test
source ./local/py3/bin/activate
pip uninstall pyyaml
make pyenvinstall

Author's checklist

Related issues

dalf · 2021-01-29T11:03:00Z

From #2514 (comment) :

Do you know why it crashes now and how we can fix it?

The cache contains a copy './local'.
It is not possible to clear the cache: actions/cache#2 (comment) (see the top of the page)

It seems the cache is corrupted.

the directory contains ./local without proper initialization.
so make pyenv does nothing since the ./local exists.
so make pyenvinstall fails because pyyaml is not installed (since ./setup.py requires pyyaml)

dalf · 2021-01-29T12:02:57Z

It should fix the CI.

@return42 can you give a look ?

return42

See my comments. All in all it LGTM .. there are two things that bother me a bit.

I made some test and killed the make clean pyenv to get a corrupt /local .. the pyyaml problem still exists under some circumstance.
makefile.python is abstract, only and handles common python, pip and setuptools issues. This patch adds some pyyaml aspects and is not an abstract solution.

At the moment I don't have more time and I also don't have a better solution right now. Is it OK for you to wait with merge. Otherwise I can also think about a more abstract solution afterwards.

utils/makefile.python

return42 · 2021-01-29T12:23:51Z

utils/makefile.python

@@ -119,13 +120,13 @@ quiet_cmd_pyenvuninstall = PYENV     uninstall   $2
 # $2 path to folder where virtualenv take place
 quiet_cmd_virtualenv  = PYENV     usage: $ source ./$@/bin/activate
      cmd_virtualenv  = \
-	if [ ! -d "./$(PY_ENV)" ];then                                  \
-		$(PYTHON) -m venv $(VTENV_OPTS) $2;                     \
+	if [ -d "./$(PY_ENV)" -a -x "./$(PY_ENV_BIN)/python" ]; then \


Checking the existence of ./$(PY_ENV_BIN)/python" makes also sense to me. This might help in cases where a previous build of the virtualenv has been failed for any reason.

I thought twice about a more robust criteria but I don't have anything better to suggest.

Another thought: now, with removing $(PY_ENV)/requirements.sha256 before running pip install and the && sha256sum requirements*.txt > $(PY_ENV)/requirements.sha256 from line 104-105, checking existence of $(PY_ENV)/requirements.sha256 could be a good criteria here?

Is there a case where the pyenv target is invoked but not the pyenvinstall target ?

if no, why there are two targets ?

if yes, then we can't $(PY_ENV)/requirements.sha256 : each invocation of pyenv will reinstall the virtualenv.

These makefiles has been implemented for general use cases. I use them in several projects as boilerplate. The motivation was to have typical development workflows localy at hand, build on simple tools available nearly everywhere.

Back to your question why there are two targets ?

pyenv: builds the environment for development / typically requirements.txt
penvinstall: installs the python package in this environment (pip install -e) / typically setup.py

In the searx project requirements.txt and setup.py include identical requirements but this is not typical for python projects. Beside this you can also think about projects containing more than one package.

Is there a case where the pyenv target is invoked but not the pyenvinstall target ?

With the words above, my answer is: it depends .. But from makefile.python's POV the answer is : yes

Your questions helped me to sort my thoughts, but for today I am to tired .. tomorrow I have more time, then I'll think about it again ..

pyyaml in setup.py

each invocation of pyenv will reinstall the virtualenv

dalf · 2021-01-29T13:25:25Z

makefile.python is abstract

Abstract version:

replace $(PY_ENV_BIN)/python -m pip $(PIP_VERBOSE) install $$(grep pyyaml ./requirements.txt);
by $(PY_ENV_BIN)/python -m pip $(PIP_VERBOSE) install -r ./requirements-presetup.txt;
requirements-presetup.txt contains only one line pyyaml==5.4.1 (to content has to be synchronized with requirements.txt).

But:

If we could avoid to another requirements*.txt, it would be nice
I prefer to find a way to remove the pyyaml dependency (related to [mod] move brand options from Makefile to settings.yml #2408 )
Is it a problem if utils/makefile.python is not abstract ?

return42 · 2021-01-29T14:04:47Z

utils/makefile.python

-		$(PY_ENV_BIN)/python -m pip $(PIP_VERBOSE) install -e $2$(PY_SETUP_EXTRAS) ;\
+		rm -f $(PY_ENV)/requirements.sha256; \
+		$(PY_ENV_BIN)/python -m pip $(PIP_VERBOSE) install $$(grep pyyaml ./requirements.txt); \
+		$(PY_ENV_BIN)/python -m pip $(PIP_VERBOSE) install -e $2$(PY_SETUP_EXTRAS) &&\


Thought: if I ignore the git workflow caching issue and have a complete new look at this two lines, I cant understand why line 103 is needed .. or did I lost the overview?

searx/setup.py

Line 11 in 71d6697

from searx import brand

brand is a variable initialized in searx/__init__.py :

searx/searx/__init__.py

Line 27 in 71d6697

settings, settings_load_message = searx.settings_loader.load_settings()

searx/searx/settings_loader.py

Line 8 in 71d6697

import yaml

Sorry I am an idiot .. I totally forgot the main problem

Hm .. after thinking again .. pyyaml should already be installed when the virtualenv is build

https://github.com/dalf/searx/blob/fe3085d68e965732ca7291a14225b34a7e046e92/utils/makefile.python#L122-L129

and this will be installed before pyenvinstall is called:

https://github.com/dalf/searx/blob/fe3085d68e965732ca7291a14225b34a7e046e92/utils/makefile.python#L193

In #2517 (comment) I described it:

pyenv: builds the environment for development / typically requirements.txt
penvinstall: installs the python package in this environment (pip install -e) / typically setup.py

The other point you asked:

each invocation of pyenv will reinstall the virtualenv

Our mistake was to build sha256sum requirements*.txt > $(PY_ENV)/requirements.sha256 ;\ in cmd_pyenvinstall .. this optimizing should take place in cmd_virtualenv.

Or I am wrong? .. I will give it a try ...

Hm .. after thinking again .. pyyaml should already be installed when the virtualenv is build

cmd_virtualenv seems to fail sometimes on github action:

in this case, the ./local contains an initialized virtualenv, but without pyyaml

cmd_virtualenv won't detect that pyyaml is missing (or other dependencies): the virtualenv exists, ...python is an executable.

then cmd_pyenvinstall fails because pyyaml is missing.

the cache is updated with a broken virtualenv.

all new builds on github action fail.

An alternative implementation (without ...grep pyyaml ./requirements.txt)... ) :

after $(PY_ENV_BIN)/python -m pip install $(PIP_VERBOSE) -r requirements.txt;, a file is created (or just a copy of requirements.txt)

if [ -d "./$(PY_ENV)" -a -x "./$(PY_ENV_BIN)/python" ]; then also check if this file exists.

basically it duplicates if ! cat $(PY_ENV)/requirements.sha256 2>/dev/null | sha256sum --check --status 2>/dev/null; then in cmd_virtualenv.

I work on a solution .. give me some time this day and I will show you my solution:

An alternative implementation
...
..

All this goes in the right direction to hardening, but instead hardening the installation, we have to hardening the venv build .. I will consider it in my solution / thanks!

@dalf I implemented a alternative PR #2524 ... its all described there ..

aka: ensure that 'make test' works as expected The cache contains a copy './local' which is - under some circumstance - corrupted. It is not possible to clear the cache [1] (see the top of the page). Ensure that 'make test' works as expected [2] even if - the python interpreter is missing - the virtualenv exists but pyyaml is missing To hardening when the workflow cache fails, this patch adds the new target 'travis.test' into the workflow. This target probes to import a python module 'yaml'. If this fails the virtualenv will be completely new build. [1] actions/cache#2 (comment) [2] searx#2517 (comment) Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>

"make pyenv" ensures that ./local/py3/bin/python is an executable

Target pip-exe is a prerequisite of the targets: - pyinstall - pyuninstall and was accidentally deleted in commit 9b48ae4. HINT: do not confuse pyinstall with penvinstall pyinstall & pyuninstall Installing into user's HOME using pip from OS, therefore the message is needed. pyenvinstall & pyenvuninstall Installing into virtualenv (./local) using pip which is provided by prerequisite 'pyenv' in the virtualenv. Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>

aka: ensure that 'make test' works as expected The cache contains a copy './local' which is - under some circumstance - corrupted. It is not possible to clear the cache [1] (see the top of the page). Ensure that 'make test' works as expected [2] even if - the python interpreter is missing - the virtualenv exists but pyyaml is missing To hardening when the workflow cache fails, this patch adds the new target 'travis.test' into the workflow. This target probes to import a python module 'yaml'. If this fails the virtualenv will be completely new build. [1] actions/cache#2 (comment) [2] searx#2517 (comment) Signed-off-by: Markus Heiser <markus.heiser@darmarit.de>

dalf force-pushed the debug-ci branch 7 times, most recently from f809980 to 577ab86 Compare January 29, 2021 09:08

dalf force-pushed the debug-ci branch 4 times, most recently from ca40751 to c49efd2 Compare January 29, 2021 11:49

dalf changed the title ~~[DEBUG] CI: Add pip freeze~~ Update pyenv pyenvinstall Make targets Jan 29, 2021

dalf force-pushed the debug-ci branch from c49efd2 to 288b222 Compare January 29, 2021 12:01

dalf marked this pull request as ready for review January 29, 2021 12:03

return42 previously approved these changes Jan 29, 2021

View reviewed changes

dalf dismissed return42’s stale review via fe3085d January 29, 2021 13:05

dalf force-pushed the debug-ci branch from 288b222 to fe3085d Compare January 29, 2021 13:05

return42 reviewed Jan 29, 2021

View reviewed changes

return42 mentioned this pull request Jan 30, 2021

Hardening travis 'Run tests' workflow when git-workflow caching fails #2524

Closed

dalf force-pushed the debug-ci branch from fe3085d to 87ac530 Compare February 1, 2021 15:55

dalf and others added 3 commits February 1, 2021 16:58

[mod] more robust make pyenv / make pyenvinstall

d70c5a6

"make pyenv" ensures that ./local/py3/bin/python is an executable

dalf force-pushed the debug-ci branch from 87ac530 to 8c45f11 Compare February 1, 2021 15:58

dalf merged commit 0a8799b into searx:master Feb 1, 2021

dalf deleted the debug-ci branch February 1, 2021 16:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pyenv pyenvinstall Make targets #2517

Update pyenv pyenvinstall Make targets #2517

dalf commented Jan 29, 2021 •

edited

dalf commented Jan 29, 2021

dalf commented Jan 29, 2021

return42 left a comment •

edited

return42 Jan 29, 2021 •

edited

return42 Jan 29, 2021

dalf Jan 29, 2021 •

edited

return42 Jan 29, 2021 •

edited

dalf commented Jan 29, 2021 •

edited

return42 Jan 29, 2021 •

edited

dalf Jan 29, 2021 •

edited

return42 Jan 29, 2021

return42 Jan 30, 2021

dalf Jan 30, 2021

return42 Jan 30, 2021

return42 Jan 30, 2021

	- name: Cache Python dependencies
	id: cache-python
	uses: actions/cache@v2
	with:
	path: ./local
	key: python-${{ matrix.os }}-${{ matrix.python-version }}-${{ hashFiles('requirements*.txt', 'setup.py') }}
	- name: Install Python dependencies
	if: steps.cache-python.outputs.cache-hit != 'true'

Update pyenv pyenvinstall Make targets #2517

Update pyenv pyenvinstall Make targets #2517

Conversation

dalf commented Jan 29, 2021 • edited

What does this PR do?

Why is this change important?

How to test this PR locally?

Author's checklist

Related issues

dalf commented Jan 29, 2021

dalf commented Jan 29, 2021

return42 left a comment • edited

Choose a reason for hiding this comment

return42 Jan 29, 2021 • edited

Choose a reason for hiding this comment

return42 Jan 29, 2021

Choose a reason for hiding this comment

dalf Jan 29, 2021 • edited

Choose a reason for hiding this comment

return42 Jan 29, 2021 • edited

Choose a reason for hiding this comment

dalf commented Jan 29, 2021 • edited

return42 Jan 29, 2021 • edited

Choose a reason for hiding this comment

dalf Jan 29, 2021 • edited

Choose a reason for hiding this comment

return42 Jan 29, 2021

Choose a reason for hiding this comment

return42 Jan 30, 2021

Choose a reason for hiding this comment

dalf Jan 30, 2021

Choose a reason for hiding this comment

return42 Jan 30, 2021

Choose a reason for hiding this comment

return42 Jan 30, 2021

Choose a reason for hiding this comment

dalf commented Jan 29, 2021 •

edited

return42 left a comment •

edited

return42 Jan 29, 2021 •

edited

dalf Jan 29, 2021 •

edited

return42 Jan 29, 2021 •

edited

dalf commented Jan 29, 2021 •

edited

return42 Jan 29, 2021 •

edited

dalf Jan 29, 2021 •

edited