refactor: drop TVM for cleaner implementation in isolated environment #25
Conversation
mariajgrimaldi
force-pushed
the
MJG/use-bash-default-shell
branch
2 times, most recently
from
e71f64f to
fc2ed6b
Compare
mariajgrimaldi
force-pushed
the
MJG/remove-tvm
branch
2 times, most recently
from
e8566bb to
e803e74
Compare
mariajgrimaldi
force-pushed
the
MJG/remove-tvm
branch
from
e803e74 to
1c76a2b
Compare
mariajgrimaldi
force-pushed
the
MJG/remove-tvm
branch
from
1c76a2b to
6da1192
Compare
.github/workflows/build.yml
Outdated
| # This command copies all the files and folders | ||
| # from the repository STRAIN_PATH to the recently above created TVM PROJECT folder | ||
| cp -r $(ls --ignore=$TUTOR_APP_NAME) $TUTOR_APP_NAME/ | ||
| pip install tutor[full]==$TUTOR_VERSION |
There was a problem hiding this comment.
This is not a blocker comment, but, taking advantage of the fact we are no longer using tvm, why not make a pip install in the form of git, to allow nightly builds?
Example: pip install git+git@github.com:overhangio/tutor.git@$TUTOR_VERSION
I'm not sure what is the demand for nightly builds, but if we have some, we can use the git version. If not, leave it as it is because it is more explicit.
There was a problem hiding this comment.
I'll do some testing to see if we can do the same there. Thanks for the suggestion!
There was a problem hiding this comment.
I forgot to say, but I had already applied the suggestion: 9176b11. Thanks again!
| env: | ||
| REQUIRED_KEYS: TUTOR_VERSION TUTOR_APP_NAME | ||
| REQUIRED_KEYS: TUTOR_VERSION |
There was a problem hiding this comment.
If we no longer require this variable, we should, as future work, remove that variable from the documentation of PICASSO: https://internal.docs.edunext.co/en/latest/internal/Products_and_Services/by_products/picasso/picasso-v2.html?highlight=picasso%20v2#requirements
There was a problem hiding this comment.
I would say let's do it as soon as possible so we always keep the documentation updated
There was a problem hiding this comment.
I don't have blocker comments, and the workflow works (tested here: https://github.com/eduNEXT/ednx-strains/actions/runs/11135119998).
Extra note: I like that we remove some code by adding the env variables.
Thanks for this ✨
Description
This PR drops the usage of TVM to leverage the fact that we're running on isolated environments (GH hosted runners) and don't need the overhead of installing libraries for this purpose. In the foreseeable future, we might consider returning to using TVM if we adopt building images with a sharing resources approach, as with Jenkins. Still, with the current state of the implementation, the easiest maintainability route is to drop it altogether.
How to test
ednx-strains/.github/build.ymlworkflow execution, go to Actions > Build Open edX strain, fill in the necessary configuration, please use the workflow version fromMJG/remove-tvm. For the strain to build, I suggest using theMJG/remove-tvmstrain branch to avoid overriding the current image on dockerhub.