-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[CI] Separate out microTVM demo script #10003
Conversation
The initial solution to running the demo in CI was to include it as part of `task_cpp_unittest.sh` - this patch aims to clean up the CI scripts to run the demo so it's ran as part of a clear script and uses `tvmc` as a user would.
@@ -39,3 +39,6 @@ python3 -m pip install --user tlcpack-sphinx-addon==0.2.1 synr==0.6.0 | |||
|
|||
# Ensure no stale pytest-results remain from a previous test run. | |||
(cd build && rm -rf pytest-results) | |||
|
|||
# Install tvm as a local package to allow tvmc to work | |||
python3 -m pip install --user -e ./python |
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.
i think this would install tvm in .local
, i wonder if it would cause some staleness issues for folks running this script locally and invoking import tvm
(e.g. would it copy all the .pyc files into ~/.local/lib/pythonXX/tvm
)? maybe a better strategy is to python3 -mtvm.driver.tvmc
in run_demo.sh
above, and just comment that this way is used to avoid these problems?
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.
oh apologies--i noticed the -e
flag now. however, i still wonder if it's wise to create two import paths for tvm.
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.
maybe a better strategy is to python3 -mtvm.driver.tvmc in run_demo.sh above, and just comment that this way is used to avoid these problems?
This is what we currently have and what I'm hoping to fix as we shouldn't be exposing any implementation detail of TVM to a user as part of running a tutorial or demo. Given this is ran in task_ci_setup.sh
, shouldn't this only be ran as part of CI and we should fix CI not using a clean environment if that's a concern?
Another potential way is we can hide it with something like alias tvmc="python3 -mtvm.driver.tvmc"
in the CI script which at least hides the CI setup from the user? Thoughts on that approach @areusch ?
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.
hmm. we could do this, but we'd have to fix this hack at least for the purposes of running the demo. I guess the best way would be to remove the PYTHONPATH override in task_demo_microtvm.sh
.
i agree we need a venv to accurately reproduce the real-world envrionment where TVM is installed. our tests should also all probably run against that venv rather than with the PYTHONPATH hack. it seems like for this PR, that would be sufficent.
Project API kind of tries to address this as well--to separate the Python deps needed to interact with a platform from those needed for TVM. we could solve this problem once for apps/microtvm projects and port this to use Project API, but that does come with, well, porting this to Project API. and, there's more to that, as there would need to be some way to specify "use tvm, but the version being tested in CI, not the one from 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.
I don't think the Project API addresses this need, as these demos represent a user with an existing application and integration with just the model rather than generating a full project they want to orchestrate from TVM. Porting to the Project API would then be unfamiliar territory to those users? As such I think we can just create a virtual env to use for running the demos in?
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.
yeah the thing i wanted to point out is that when there is a PYTHONPATH override, the venv may not be effective. you'll at least need to remove that override.
if you go this route (venv + explicitly clear PYTHONPATH for this one test), can we file a cleanup issue to run all of the pytests in a venv with TVM properly installed using pip?
It has been a while since this PR was updated, @areusch please leave a review or address the outstanding comments. @Mousius if this PR is still a work in progress, please convert it to a draft until it is ready for review. |
@Mousius closing this since it's stale. perhaps we can revisit after the Python CI deps change lands? |
The initial solution to running the demo in CI was to include it as part of
task_cpp_unittest.sh
- this patch aims to clean up the CI scripts to run the demo so it's ran as part of a clear script and usestvmc
as a user would.