Add fallback-mode option for subset#1308
Conversation
When the subset API errors or returns isBrainless=true, users can now opt into 'stop' (exit non-zero to halt CI) or 'random-sample' (split locally using --target) instead of the default run-all behavior. The option is hidden and off by default so existing pipelines are unaffected.
| elif self.fallback_mode == FallbackMode.RANDOM_SAMPLE: | ||
| target_fraction = float(self.target) if self.target is not None else 1.0 | ||
| click.echo( | ||
| "Warning: Smart Tests could not retrieve a subset. Falling back to local random sample.", |
There was a problem hiding this comment.
Since users may use options other than --target e.g.) --confidence, it might be helpful to include the target percentage in the warning message as well.
Or, if --fallback_mode is specified and the user is using an option other than --target, we could return an error in the validation arera
There was a problem hiding this comment.
sounds prudent. Let me consider
| sampled = random.sample(test_paths, min(count, len(test_paths))) | ||
| sampled_set = {id(t): t for t in sampled} | ||
| rest = [t for t in test_paths if id(t) not in sampled_set] | ||
| return cls(subset=sampled, rest=rest, subset_id='', summary={}, is_brainless=False, is_observation=False) |
There was a problem hiding this comment.
Is is_brainless=False intentionally?
There was a problem hiding this comment.
Yes, is_brainless=False is intentional. The random sample result is locally generated, not from the brainless model. I hope the behavior matches with this implementation ea8477a
There was a problem hiding this comment.
OK 👍
Could you add a comment that why we update the brainless mode value from true to false here?
I misunderstood the data flow, you don't have to add comment, sorry
| result = self.cli(*self._subset_args(rest_file.name, ("--fallback-mode", "random-sample")), mix_stderr=False) | ||
| self.assert_success(result) | ||
| all_tests = result.stdout.strip().split("\n") + Path(rest_file.name).read_text().strip().split("\n") | ||
| all_tests = [t for t in all_tests if t] |
There was a problem hiding this comment.
This is a filter. "".split("\n") returns [""] rather than []. So if either stdout or the rest file is empty, the concatenation produces empty strings and the count assertion would be wrong.
Probably, there is a better way. I'll consider again.
There was a problem hiding this comment.
I treated the empty string as the edge case but let me drop the filter for the test for simplicity 0451d3a
| # TODO(Konboi): split subset isn't provided for smart-tests initial release | ||
| # if split: | ||
| # click.echo("subset/{}".format(subset_result.subset_id)) | ||
| if subset_result.is_brainless: |
There was a problem hiding this comment.
In Brainless mode, we're already splitting requests between subset and rest on the server side.
I can understand the stop behavior, but for sampling, do you mean re-sampling the tests on the CLI side?
There was a problem hiding this comment.
yeah, you are right. this is redundant if we split tests on server.
Konboi
left a comment
Konboi
left a comment
There was a problem hiding this comment.
LGTM
I left a comment but it's nits comment so I'll approve this PR
2 participants
Background
When the subset API errors or returns isBrainless=true, users can now opt into 'stop' (exit non-zero to halt CI) or 'random-sample' (split locally using --target) instead of the default run-all behavior. The option is hidden and off by default so existing pipelines are unaffected.
Changes
--fallback-modeoption for subset command