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

[Relay] Add tests in CI which run tests based on specific Relay feature flags #4348

Closed
2 tasks
gussmith23 opened this issue Nov 15, 2019 · 5 comments
Closed
2 tasks

Comments

@gussmith23
Copy link
Contributor

A persistent problem in Relay is what @MarisaKirisame calls the "split-brain" problem: the developers of Relay and the users of Relay are of two minds on how Relay works. Specifically, Relay developers want to support advanced features that didn't exist in NNVM, while Relay users often treat Relay as NNVM. For example, Relay users will write Relay passes that work like NNVM over Relay features that also existed in NNVM, and will break/behave incorrectly over new Relay features.

In response to this problem, @MarisaKirisame has added a feature manager, which users can use to check the features of a Relay program. The associated issue also discusses modifying the pass manager such that each pass would need to specify what features a pass can handle, what features a pass may add, and which features a pass may remove.

We'd like to start adding tests into the CI which use Relay's feature sets to test newly added passes. We will test new passes on Relay programs with a variety of feature sets, to test whether they break programs which they claim they should support, given their feature set. New passes will only be tested on Relay programs with features they explicitly support. These new CI tests should help us ensure that pass developers appropriately mark which features their passes support and don't support.

Task list

  • Collect programs which have different feature sets
    • list features here...
  • Create infrastructure for testing all new passes
@gussmith23
Copy link
Contributor Author

For the record, we want to test against real models -- not just generating adversarial programs.

@gussmith23
Copy link
Contributor Author

Talking with Steven and Marisa:

  • Tests should be automatically run over all passes, so that we don't have to rely on submitters/reviewers to add new passes to existing tests
    • What's a clean way to get all of the passes automatically?
  • Passes need to be modified to include the three fields that Marisa describes in the original PR: supported features, added features, removed features
  • Passes need to be modified with machinery to check the three fields above
  • Existing passes need to be modified to state what features they support

@slyubomirsky
Copy link
Contributor

slyubomirsky commented Nov 15, 2019

I think you can iterate through the passes by looking at the defined members of tvm.relay.transform and having exceptions for the members that are not passes. This way, if anyone adds a pass to that package, they would have to hardcode an exception and should have a justification for it (we can put a comment in there to remind reviewers of that)

I.e., dir(tvm.relay.transform)

@zhiics
Copy link
Member

zhiics commented Nov 16, 2019

@gussmith23 Thanks for the proposal. I have a few thoughts/questions.

  • For the passes, can we just glob them from the registry?
  • How do you plan to represent these meta data? Are we going to just add them to PassInfo?
  • Do we want to save the states the resultant passes, i.e. if the program changes?

@gussmith23
Copy link
Contributor Author

  • Yeah, it seems like Steven's idea would work here. Just glob them from the transform module.
  • In [RFC][Relay] Feature Manager #3236, Marisa writes "we can also bake this into the pass manager: each pass will have to specify three more argument: FeatureSet input_features, FeatureSet add_features, FeatureSet remove_features, that denote which feature can this pass handle, which feature it may add, and which feature it will remove." From my understanding of the pass manager code, it seems like PassInfo might be the most logical place to add these things.
  • Are you asking whether we'll need to save the state of the passes, to check whether they abide by the add_features/remove_features they've specified? If so, we might start with the easiest solution, which would be to not save the before/after state of the pass, and not check whether it abides by its claimed add_features/remove_features. Instead, we could just use this meta-data in the CI tests that we create. These CI tests will inspect a pass, get its input_features/add_features/remove_features, and select test programs with the appropriate features. It'll run the passes over those test programs and ensure that add_features/remove_features is not violated. Thus, the running of the passes themselves, over normal code, in the normal compilation process, won't be affected, and this added metadata will only be used by our new test suite.

@areusch areusch added the needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it label Oct 19, 2022
@areusch areusch added dev:test-infra and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Nov 16, 2022
@tqchen tqchen closed this as completed Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants