-
Notifications
You must be signed in to change notification settings - Fork 1.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
Group agg rework #1741
base: main
Are you sure you want to change the base?
Group agg rework #1741
Conversation
…oup aggregate in general
…rness into group-agg-rework
@haileyschoelkopf I've only added the |
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.
Very close to done!
Left some comments, and messaged you re: how we should perhaps change the GroupConfig interface
Other than that:
- we should go through and determine which of existing groups should be aggregating and which should not. I can do this
- I need to go through again more finely the evaluator.py changes, there's quite a lot of logic in there at this point that I want to look more closely at
- We should couple this with a readme update mentioning this change + referring people to Discord and the docs files for better understanding what's affected.
Will try to aim for having this merged by end of next week. Sorry for all the delays!
@@ -242,7 +242,7 @@ def get_original(self, newarr): | |||
return res | |||
|
|||
|
|||
def make_table(result_dict, column: str = "results", sort_results: bool = True): | |||
def make_table(result_dict, column: str = "results", sort_results: bool = False): |
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.
reason for not sorting by default?
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.
It messes up the group name, as in the group name also gets sorted.
@@ -0,0 +1,45 @@ | |||
group: test-1 |
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.
is there an assorted test for this? Note also #1916 should help with PRs like this
Co-authored-by: Hailey Schoelkopf <[email protected]>
Co-authored-by: Hailey Schoelkopf <[email protected]>
Co-authored-by: Hailey Schoelkopf <[email protected]>
group_config
will need to be defined in the yaml that consists ofaggregate_metric
(True/False, default False) andweight_by_size
(True/False, default False).task_id
inConfigurableGroup
andConfigurableTask
to be used as identifier in lieu of task name/group name.