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

Consenus mode #42

Merged
merged 5 commits into from
May 29, 2024
Merged

Consenus mode #42

merged 5 commits into from
May 29, 2024

Conversation

ravyu-jump
Copy link
Contributor

@ravyu-jump ravyu-jump commented May 28, 2024

Adds a "consensus mode" flag which, among other things, normalizes error codes between effects. This way, tests do not fail in scenarios that don't break consensus (i.e., different error codes)

Also did some cleanup by removing check_consistency_in_results in multiprocessing_utils.. We have a lot more cleaning up to do

src/test_suite/test_suite.py Outdated Show resolved Hide resolved
src/test_suite/test_suite.py Outdated Show resolved Hide resolved
if consensus_mode:
original_diff_effects_fn = globals.harness_ctx.diff_effect_fn

def diff_effect_wrapper(a, b):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this take into account scenarios where there are multiple fields that need to be normalized? For example, for InstrEffects there's error codes and custom error codes that should both be ignored

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this stuff is hardcoded unfortunately. Didn't see any output in custom_err so I kinda ignored it for now.

Wasn't really sure of how to deal with different effects having different fields to ignore in a consensus mode run. The fact that we want to modify things in place for output also complicates things quite a bit. Perhaps we can define a separate diff_effects_consensus_fn as a part of the interface?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about we make globals.harness_ctx.result_field_names a list and iterate over it with a for loop within this function? Then for example if someone wanted to find the passing cases if you ignore, for example, CU's, then they could just add that to the list. You can keep the list empty as default, and someone who wants to modify the diff behavior can just add the ignored fields to the list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I don't exactly just ignore the result fields themselves, just that if they both have error codes. So a generic "ignore fields" list won't apply

@ravyu-jump ravyu-jump merged commit 710b9ce into main May 29, 2024
1 check passed
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.

2 participants