Skip to content

Conversation

@agoston-mc
Copy link
Contributor

Add an NNEF frontend to Relax and Relay, as proposed in RFC #108.

The Docker image scripts and test scripts have also been extended to accommodate NNEF.



def verify_model(
model_path,
Copy link
Member

@tqchen tqchen May 31, 2024

Choose a reason for hiding this comment

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

Historically we test correctness via the execution results. This however, leads to compound running time of the tests, and less unit testcases. As such tests needs to run for frontend/backend pairs.

As the intermediate operators getting stablizes, we are getting to a world where we can unit test the frontend conversion independently. By validating they goes into the right structural form of IR. The backend correctness are then tested separately.

So we would now instead move toward structural equality tests for unit tests when bringing in new frontends, to reduce the cost in that front. See the FX importer for an example.
https://github.com/apache/tvm/blob/main/tests/python/relax/test_frontend_from_fx.py#L39

The execution tests can still be used, please bring them to python/nightly/frontend, so they won't be triggered per PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I also add structural tests for Relay, or would you prefer Relax only? In that case execution tests would exist for both frontends under tvm-root/tests/python/nightly/frontend/, and the structural Relax tests would be kept in the normal folder, tvm-root/tests/python/relax/.
If structural Relay tests are preferable, what IRModule creating method is recommended for Relay creation? Building a relay.Function, and comparing that with the frontend's module? Can I follow this example, and compare a span filled, with a non-span filled graph? https://github.com/apache/tvm/blob/main/tests/python/frontend/pytorch/test_fx_quant.py#L42

Also, how should I create the directory structure under nightly/frontend? Should I separate Relay from Relax under a similar structure to regular tests? Or do you have a system in mind?

Copy link
Member

Choose a reason for hiding this comment

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

if we want to keep execution tests in relay, please put them under nightly/frontend. Since relay creation can be harder

For relax, we can do structure test

Copy link
Contributor Author

@agoston-mc agoston-mc Jun 7, 2024

Choose a reason for hiding this comment

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

Done in 944db77

@agoston-mc agoston-mc requested review from tqchen and yongwww July 1, 2024 14:13
@agoston-mc
Copy link
Contributor Author

Can I ask for a status update on this PR?
I have resolved the requested changes; the checks will fail as long as the Docker image does not contain the NNEF package.

@tqchen @yongwww

@yongwww
Copy link
Member

yongwww commented Sep 12, 2024

@agoston-mc It would be good to submit the Docker-related changes (the changes under directory docker/ in this PR) as a separate PR and get it merged first. Then, we can use the newly built image in this PR to pass the CI checks. You can find some image doc here: https://tvm.apache.org/docs/contribute/ci.html?highlight=update%20container#docker-images.

agoston-mc and others added 4 commits November 28, 2024 15:45
# Conflicts:
#	ffi/scripts/run_tests.sh
#	python/tvm/meta_schedule/post_optimization/__init__.py
#	python/tvm/meta_schedule/post_optimization/nnef.py
#	python/tvm/meta_schedule/post_optimization/nnef_ops.py
#	tests/scripts/task_python_frontend.sh
#	tests/scripts/task_python_nightly.sh
@agoston-mc
Copy link
Contributor Author

@tvm-bot rerun

@agoston-mc
Copy link
Contributor Author

agoston-mc commented Jun 6, 2025

Hi,
Can I get an ETA for the reviews, as the CI checks ran fine, and the CR has been resolved?
@tqchen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants