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

Convert to pyproject.toml #2411

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Conversation

VannTen
Copy link
Member

@VannTen VannTen commented Jan 16, 2023

No description provided.

@VannTen VannTen requested review from goern and sesheta as code owners January 16, 2023 16:24
@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 16, 2023
@VannTen
Copy link
Member Author

VannTen commented Jan 16, 2023

related : thoth-station/nepthys#98

@goern
Copy link
Member

goern commented Jan 17, 2023

/assign @VannTen
@VannTen could you please check why aicoe-ci/build-check failed?

@VannTen
Copy link
Member Author

VannTen commented Jan 17, 2023 via email

@VannTen
Copy link
Member Author

VannTen commented Jan 17, 2023

Looks like the s2i/assemble script does not install the local package (adviser) without a setup.py... Something to do with micropipenv I think

@VannTen
Copy link
Member Author

VannTen commented Jan 17, 2023

/retest aicoe-ci/build-check

@sesheta
Copy link
Member

sesheta commented Jan 17, 2023

@VannTen: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

  • /test pre-commit
  • /test thoth-pytest-py38

Use /test all to run all jobs.

In response to this:

/retest aicoe-ci/build-check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@VannTen
Copy link
Member Author

VannTen commented Jan 17, 2023

From the s2i image:

if [[ ( -f setup.py || -f setup.cfg ) && -z "$DISABLE_SETUP_PY_PROCESSING" ]]; then
  echo "---> Installing application ..."
  pip install .
fi

@VannTen
Copy link
Member Author

VannTen commented Jan 17, 2023

sclorg/s2i-python-container#575

Looks like s2i-python upstream can work in that scenario. Maybe we can upgrade our version in thoth-s2i ?

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm

one question

pyproject.toml Outdated
]
classifiers = [
"Development Status :: 4 - Beta",
"Programming Language :: Python :: 3.7",
Copy link
Member

Choose a reason for hiding this comment

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

shall we change this to 3.8

python_version = "3.8"

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Jan 17, 2023
@VannTen
Copy link
Member Author

VannTen commented Jan 18, 2023 via email

@frenzymadness
Copy link

Projects with pyproject.toml should now work in Python containers as well, see: https://github.com/sclorg/s2i-python-container/blob/92131e21ddc95cc25317333f0a1e62a821dfbd56/3.11/s2i/bin/assemble#L93-L96

@VannTen
Copy link
Member Author

VannTen commented Jan 18, 2023

Yeah I've seen that. Unfortunately Thoth s2i images are a bit behind on upstream s2i :/

@sesheta sesheta removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2023
@harshad16
Copy link
Member

Possibly, I'm not too sure of what Python version we support

Atleast we should change it from 3.7 to 3.8
as this repo is built on python 3.8

python_version = "3.8"

and on thoth side, the serving is for 3.8,3.9,3.10 based on different OS envs.

@harshad16
Copy link
Member

/retest

@harshad16
Copy link
Member

can we have a rebase, so we can merge this

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

alll looks good
though the build keeps failing at this

thoth-adviser validate-prescriptions prescriptions/ --output ../prescriptions.pickle

the thoth-adviser installation generated from pyproject, is not showing the command.
still trying to find out the reason
/hold
till build is fixed

@sesheta sesheta added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2023
@VannTen
Copy link
Member Author

VannTen commented Jan 27, 2023

/retest-required

@VannTen
Copy link
Member Author

VannTen commented Jan 27, 2023

Oh yeah I think I know why. the /usr/libexec/s2i/assemble is the one from thoth-s2i. Hence it does not use pip install . since it does not detect a setup.py or setup.cfg -> python package is not installed, thus, thoth-adviser command is not present. Should we switch to the upstream s2i for adviser right now ?

#2411 (comment)

Content of the s2i assemble on s2i-thoth
$ podman run -ti s2i-thoth-ubi8-py38:v0.35.1  bash

(app-root) cat /usr/libexec/s2i/assemble 
#!/bin/bash
#!/usr/bin/env bash
#
# ------------------------>% Thoth related content %<------------------------
#
THOTH_ASSEMBLE_DEBUG=${THOTH_ASSEMBLE_DEBUG:-0}
[[ ${THOTH_ASSEMBLE_DEBUG} -eq 0 ]] || set -e

# Submit stack to Thoth, but do not use the recommended one:
THOTH_DRY_RUN=${THOTH_DRY_RUN:-0}
# Force advises even if the lock is present in the repo:
THOTH_ADVISE=${THOTH_ADVISE:-1}
# Trigger Thoth configuration file check:
THOTH_CONFIG_CHECK=${THOTH_CONFIG_CHECK:-1}
# Use provenance checks by default if THOTH_ADVISE is set to 0.
THOTH_PROVENANCE_CHECK=${THOTH_PROVENANCE_CHECK:-1}
# Use Thamos from git instead of a PyPI release:
THOTH_FROM_MASTER=${THOTH_FROM_MASTER:-0}
# Generate .thoth.yaml file during the build process.
THOTH_FORCE_GENERATE_CONFIG=${THOTH_FORCE_GENERATE_CONFIG:-0}
# Thoth host to submit recommendations to:
export THOTH_HOST=${THOTH_HOST:-khemenu.thoth-station.ninja}
# Disable progressbar for thamos:
export THAMOS_NO_PROGRESSBAR=${THAMOS_NO_PROGRESSBAR:-1}
# Fallback to the lock file present in the repo if analysis fails.
THOTH_ERROR_FALLBACK=${THOTH_ERROR_FALLBACK:-0}
# Turn off Thoth using just one flag, only config generation is enabled.
THOTH_OFF=${THOTH_OFF:-0}

[[ ${THOTH_OFF} -ne 0 ]] && {
  echo ">>> Disabling Thoth"
  THOTH_ADVISE=0
  THOTH_PROVENANCE_CHECK=0
  THOTH_CONFIG_CHECK=0
  THOTH_DRY_RUN=1
}

# Print Thoth configuration to logs if debug is enabled.
[[ ${THOTH_ASSEMBLE_DEBUG} -eq 0 ]] || env | grep -e '^THOTH_' -e '^THAMOS_'

# A directory where s2i places sources.
pushd /tmp/src

# Make a backup of the lock present in the git root.
[[ ${THOTH_DRY_RUN} -ne 0 && -f Pipfile.lock ]] && cp Pipfile.lock ../
[[ ${THOTH_DRY_RUN} -ne 0 && -f requirements.txt ]] && cp requirements.txt ../

function restore_lock() {
  [[ -f ../Pipfile.lock ]] && cp ../Pipfile.lock .
  [[ -f ../requirements.txt ]] && cp ../requirements.txt .
}

echo ">>> Thoth s2i builder image version: ${THOTH_S2I_VERSION}"

[[ ${THOTH_FORCE_GENERATE_CONFIG} -ne 0 ]] && {
  rm -f .thoth.yaml
  thamos config --no-interactive
}

[[ ${THOTH_ADVISE} -ne 0 || ${THOTH_PROVENANCE_CHECK} -ne 0 ]] && {
  echo ">>> Performing hardware and software discovery..."
  thamos config --no-interactive || exit 1
  echo ">>> Thoth's configuration file after hardware and software discovery:"
  cat .thoth.yaml | /usr/bin/python3 -c "import yaml; import json; import sys; json.dump(yaml.safe_load(sys.stdin), sys.stdout, indent=2);"
}

[[ ${THOTH_CONFIG_CHECK} -ne 0 ]] && {
  thamos check || {
    if [[ ${THOTH_ERROR_FALLBACK} -ne 0 ]]; then
      echo ">>> Check of the configuration file failed, proceeding with the installation process based on the error fallback flag"
    else
      echo ">>> Aborting build as Thoth's configuration check failed; you can suppress the failure by providing THOTH_ERROR_FALLBACK=1" >&2
      exit 1
    fi
  }
}

[[ ${THOTH_ADVISE} -ne 0 ]] && {
  if [[ ${THOTH_FROM_MASTER} -eq 1 ]]; then
      pip3 install --force-reinstall -U git+https://github.com/thoth-station/thamos || exit 1
      pip3 install --force-reinstall -U git+https://github.com/thoth-station/invectio || exit 1
  fi

  echo -e "\n>>> Asking Thoth for advise..."
  if [[ ${THOTH_DRY_RUN} -eq 0 ]]; then
    thamos advise || {
      if [[ ${THOTH_ERROR_FALLBACK} -ne 0 ]]; then
        echo ">>> Thoth stack analysis failed with the following log:"
        thamos log
        echo ">>> Restoring previous requirements lock"
        restore_lock
      else
        if [[ -f ".thoth_last_analysis_id" ]]; then
          echo ">>> Thoth stack analysis failed with the following log:"
          thamos log
        fi

        echo ">>> Thoth stack analysis failed, this build will fail shortly; you can suppress this failure by providing THOTH_ERROR_FALLBACK=1" >&2
        exit 1
      fi
    }
  else
    thamos advise --no-wait || {
      [[ ${THOTH_ERROR_FALLBACK} -eq 0 ]] && exit 1
    }
  fi
} || {
  echo ">>> Thoth advises are not activated"
}

# Restore previous lock, do not use the original one on dry run.
[[ ${THOTH_DRY_RUN} -ne 0 ]] && {
  echo ">>> Restoring previous requirements lock as THOTH_DRY_RUN was set" >&2
  restore_lock
}

# Show lock on debug.
[[ ${THOTH_ASSEMBLE_DEBUG} -ne 0 && -f Pipfile.lock ]] && cat Pipfile.lock
[[ ${THOTH_ASSEMBLE_DEBUG} -ne 0 && -f requirements.txt ]] && cat requirements.txt

[[ ${THOTH_PROVENANCE_CHECK} -ne 0 ]] && {
  if [[ ${THOTH_ADVISE} -eq 0 && ${THOTH_DRY_RUN} -eq 0 ]]; then
    echo ">>> Expanding Thoth's configuration file..."
    thamos config --no-interactive || exit 1
    echo ">>> Thoth's configuration file after expansion:"
    cat .thoth.yaml | /usr/bin/python3 -c "import yaml; import json; import sys; json.dump(yaml.safe_load(sys.stdin), sys.stdout, indent=2);"
    echo -e "\n>>> Asking Thoth for provenance check..."
    thamos provenance-check || {
      [[ ${THOTH_ERROR_FALLBACK} -eq 0 ]] && exit 1
    }
  else
    echo ">>> Provenance checks skipped as the lock file used is from Thoth"
  fi
}

popd

# Uncomment if you want to use this script solely in an s2i as an extension:
#exec /usr/libexec/s2i/assemble
#
# ------------------------>% Thoth related content %<------------------------
#

function is_django_installed() {
  python -c "import django" &>/dev/null
}

function should_collectstatic() {
  is_django_installed && [[ -z "$DISABLE_COLLECTSTATIC" ]]
}

function virtualenv_bin() {
    # New versions of Python (>3.6) should use venv module
    # from stdlib instead of virtualenv package
    python3.8 -m venv $1
}

set -e

shopt -s dotglob
echo "---> Installing application source ..."
mv /tmp/src/* "$HOME"

# set permissions for any installed artifacts
fix-permissions /opt/app-root -P


if [[ ! -z "$UPGRADE_PIP_TO_LATEST" ]]; then
  echo "---> Upgrading pip, setuptools and wheel to latest version ..."
  if ! pip install -U pip setuptools wheel; then
    echo "WARNING: Installation of the latest pip, setuptools and wheel failed, trying again from official PyPI with pip --isolated install"
    pip install --isolated -U pip setuptools wheel
  fi
fi

thamos install

if [[ ( -f setup.py || -f setup.cfg ) && -z "$DISABLE_SETUP_PY_PROCESSING" ]]; then
  echo "---> Installing application ..."
  pip install .
fi

if should_collectstatic; then
  (
    echo "---> Collecting Django static files ..."

    APP_HOME=$(readlink -f "${APP_HOME:-.}")
    # Change the working directory to APP_HOME
    PYTHONPATH="$(pwd)${PYTHONPATH:+:$PYTHONPATH}"
    cd "$APP_HOME"

    # Look for 'manage.py' in the current directory
    manage_file=./manage.py

    if [[ ! -f "$manage_file" ]]; then
      echo "WARNING: seems that you're using Django, but we could not find a 'manage.py' file."
      echo "'manage.py collectstatic' ignored."
      exit
    fi

    if ! python $manage_file collectstatic --dry-run --noinput &> /dev/null; then
      echo "WARNING: could not run 'manage.py collectstatic'. To debug, run:"
      echo "    $ python $manage_file collectstatic --noinput"
      echo "Ignore this warning if you're not serving static files with Django."
      exit
    fi

    python $manage_file collectstatic --noinput
  )
fi

# set permissions for any installed artifacts
fix-permissions /opt/app-root -P

This is a workaround to force s2i to build the python project.
@VannTen
Copy link
Member Author

VannTen commented Jan 27, 2023

/hold
till build is fixed

Isn't the build check required anyway ? I mean, prow is not gonna merge if the build fail right ? So no need to manually hold it ?

@harshad16
Copy link
Member

Isn't the build check required anyway ? I mean, prow is not gonna merge if the build fail right ? So no need to manually hold it ?

i added hold just for others, so no one actually merges it manually.

Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/unhold

thanks 👍

@sesheta sesheta added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 27, 2023
@sesheta
Copy link
Member

sesheta commented Jan 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2023
@sesheta sesheta merged commit 0669b0e into thoth-station:master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants