-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix/ci cache #319
Fix/ci cache #319
Conversation
# todo: replace with a more sane solution | ||
- name: install tensorflow_quantum # https://www.tensorflow.org/quantum/install | ||
run: | | ||
poetry run pip install --upgrade pip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может сразу попробуем затащить tfq в poetry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да это тоже самое будет, что и это. Можно попробовать, конечно.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Нет, в том то и фишка, что это будет на 2-3 минуты быстрее. Сейчас он сначала долго ставит зависимости из поетри, а потом долго ставит их еще раз при установке tfq. Там прямо в логах видно, как он удаляет одни версии и скачивает + ставит другие.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это один раз делается для создания requirements.yml, вообще возможно там ничего не соберётся и придется вручную править зависимости 😔
label: linux-64 | ||
prefix: /usr/share/miniconda3/envs/qmlcourse.ai | ||
|
||
# - os: macos-latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Надо это удалить.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это нельзя удалять, с помощью этого префикса сделано кэширование у них:-(
Пока не уверен, что будет работать матрица без хотя бы второго значения
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я про то, что удалить тестирование сборки для других осей
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пока не узнаю, что можно задать матрицу из одного элемента удалять нельзя, просто придется иначе колхозить как-то отключение сборки для других платформ внутри кода, типа if вставить лишний.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id: mamba-cache | ||
|
||
- name: export some tokens | ||
env: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Задание этого env надо перенести в сборку книги, а сам шаг удалить.
github_token: ${{ secrets.GITHUB_TOKEN }} | ||
publish_dir: ./qmlcourseRU | ||
|
||
# - name: Build PDF file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Пока удалить
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да починим pdf совсем скоро, если все собирается
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Его нет смысла чинить в текущем виде. Надо собирать только через ковертацию в LaTeX, а там сразу будут проблемы (как минимум, с 99% вероятностью не будет русского языка из коробки и т.д.)
# poetry run jupyter-book build ./qmlcourseRU --builder pdfhtml | ||
# mv ./qmlcourseRU/_build/pdf/book.pdf ./qmlcourseRU/_build/pdf/all_book.pdf | ||
|
||
# - name: Deploy only pdf files |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Покаудалить
@@ -0,0 +1,43 @@ | |||
name: create-requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а как этот файл будет запускаться автоматом? и он вроде как дубль что в workflows
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Я так понимаю, что это правильный путь как надо сделать, и во все воркфлоу добавить if про то, что если *.lock или *.toml изменён, то запустить этот экшен. Я посмотрю как в докер билдах делают, там такая же проблема
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему «изменен»? нужно чтобы выполнялось только если оно зелёное
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
про дубль этого файл в этом pr так и не понял
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtrokhymenko один в actions, совсем правильно будет сделать через него, то есть вставить if файлы с библиотеками поменялись, тогда запустить сначала этот action. Второй файл в workflows нужен чтобы просто пока тестировать, но в целом и с его помощью можно тоже сделать, типа придется добавить тайм-аут и такое решение не очень.
Сам пока не разобрался как это делается правильно, но тут https://github.community/t/is-it-possible-to-run-the-job-only-when-a-specific-file-changes/115484 есть разные варианты
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
а как именно и часто будет запускаться файл что в actions
? и ты уверен что он там будет так работать, а то по твоей ссылки сходу не нашел что именно в той директории оно должно быть
@@ -0,0 +1,100 @@ | |||
name: new-deploy-book |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
зачем new
? просто заменяем сразу. это же касается и new-deploy-branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
добавить зависимость что это этот файл должен запускаться при условии успешного create-requirements
. ну или оттуда сюда всунуть отдельным шагом
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ещё не оттестировано, заренеймить не долго, мы же потом мерджим сквошем, будет как будто изначально меняли те файлы.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему, ты ведь создал новый файл?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Потому что сквош переписывает историю. Если будет нужно, я сделаю rebase и все будет как будто мы изначально их правили, если потребуется тестирование в мастере, я так понял, в ветках actions не подтягиваются?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ты меня уже запутал этими файлами и не понятными дублями
итого, не нужно ничего дублировать
Я вот смотрю на это все и думаю - а может перепишем все на ansible? И запускать будем через Ansible Actions. Плюсы:
Я ansible хорошо знаю и он гораздо гибче, чем GH Actions кмк |
в ansible ноль с большой дыркой |
@alexey-pronkin собираешь подступаться всё же? |
Это можно закрывать, так как деплой чутка изменился, нужно с нуля переделывать. Плюс время прошло, может уже тех проблем с совместимостью нет |
cache уже работает, после того как убрал condа |
Я все-таки вижу так, что нужно создавать requirements.yml для анаконды (сделано, но не протестировано в create-requirements.yml) и в деплое использовать ее. Единственное, что пока не сделал, это нужно create-requirements.yml перенести в экшены и добавить в деплое проверку, поменялся ли poetry.lock и pyproject.toml, если поменялись, то запускать этот экшен.
Не тестировал, не работал с gh-actions, так что если кто-то может помочь, было бы неплохо.
Вот тут дока, как делать кэш для миниконды https://github.com/marketplace/actions/setup-miniconda#caching
Критически важно ставить poetry из анакоды, типа conda install poetry, тогда не нужны все эти экшены, которые не очень понятно, как работают, изначально poetry будет ставить все в активированный environment, а эти пакеты будут при conda export идти как будто они из PYPI установлены.