-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
…ibrary Merge branch 'develop' into prism/comparators
Merge branch 'prism/comparators' of https://github.com/credo-ai/credoai_lens into prism/comparators
…tor. Some brittleness due to MetricContainer structure
- Moved to Dataframe operations - Added optional definition of the operation
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.
Left some comments. Bullet point high level critiques:
- I feel like the
compare.py
class is unneeded and can be rolled into Prism. Happy to discuss and be wrong here though. - The way a reference is defined and IDed is not flexible enough. Only works for cross-model comparisons, when really we want cross "context" comparisons. If the same model is run on different samples of data, we may want to compare those.
- The user interface of Prism, which is supposed to simplify everything, instead obfuscates.
- Documentation needed for most functions (though easy to follow anyway)
@IanAtCredo Overall responses to the high level critique:
Initially I was of the same opinion, however I feel we need this kind of objects due to the variety of tasks that Prism will have to handle in the future (not just comparisons). If everything gets put into Prism I feel it will eventually ballon up pretty quickly. I intended for Prism to only handle orchestration logic, including the translation from user input into the tasks to perform.
Agreed, this was meant as a temporary solution. I think Data/Model/SensitiveFeature define the context ATM, I wonder what is the best way of conveying this information. The simplest is a combo of Model_Name + Data_Name + Feature_Name/Value, this will get ugly pretty fast. Potential solution: leverage
Agreed, I wasn't extremely happy about it either. We can ask the user to pass the actual (task) class, like
another argument to keep the task objects 😛 . In any case, whether we map user input string to behavior, or we pass tasks directly, a certain amount of knowledge is necessary on user side. |
Few thoughts: Why does the user need to specify tasks at all? Or at the level of granularity you are describing? Why not just "here are a few models, either aggregate or compare them". I can buy that if you say "aggregate them", implicitly a list of tasks are created, but should that be the main way the user interacts? Perhaps some super-user will want specific access to the tasks (which we can allow), but that's secondary to the primary user interaction. Data/Model/SensitiveFeature as context - yeah. I think we may want to define this somewhere. I was originally thinking that a run of Lens is the context. Why do we need to infer the context? Can't we just say "we took a bucket of what you said was one thing and compared it to another." To generalize this to evidence separated from Lens is trickier within your current implementation, but could be done by a list of evidence-lists, rather than inferring separation by IDs. |
Metric comparator works correctly
@IanAtCredo Update:
|
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.
I'm not sure if changes need to be made, but I feel like the current "identifier" implementation is quite shaky and will lead to problems down the road. Let's discuss whether changes are necessary.
We can also call this an experimental feature, accept some brittleness and see if it is required by others.
@@ -35,6 +36,27 @@ def __post_init__(self): | |||
self.evaluator.metadata = self.metadata | |||
self.metadata["evaluator"] = self.evaluator.name | |||
|
|||
@property | |||
def identifier(self): |
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.
I spent a while trying to make this more robust. As it is, this seems quite brittle! We're passing an ID that is made up on-the-fly here, to be referenced via a hardcoded set of ID_Cols. Doesn't seem good.
Tried to replace with this:
@property
def id(self):
eval_info = self.evaluator.get_info()["metadata"]
return "~".join(f"{k}:{v}" for k, v in eval_info.items())
which makes use of another function we already have to create metadata (I also changed "identifier" => "id")
This results in outputs like this:
'evaluator:Performance~model_name:credit_RF1_classifier~assessment_dataset_name:UCI-credit-default-test'
This can't be used later however because the metadata isn't consistent amongst evaluators. However, I'm confused why that should be an issue. Shouldn't we be able to do comparisons evaluator-by-evaluator when they match up? The issue now is coming up because everything is being coerced into one dataframe.
I tried to remove the explicit separation of ID into ID_Cols, but then saw that the ID_Cols were used so that evaluation could be directed to be over the ref class
, like "model". I was confused about that implementation as well. Once we know that we have the same ID between two lens instances, shouldn't we be able to compare? Why do we need a ref class at all?
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.
The reference was split in ref_type
and ref
to allow the flexibility of comparing either by model or by whichever dataset (training/assessment) is available. This is to accommodate 2 type of use cases:
- Many models comparisons: multiple iterations of the same model vs initial case, or different models against a reference one.
- Multiple dataset comparisons: comparing result when dataset change, over time or due to some form of sampling for example.
If Metadata structure was always constant then I would agree that we can simply pass the self.evaluator.get_container_info()
, but as it stands I would have to still do extra steps to standardize the output in a similar way to what I am already doing.
Standardization is also good for results displaying, I think breaking down the id in labelled columns helps user understanding the results, rather then having to read a long and variable id.
be instantiated with their necessary artifacts. One or multiple Lens objects | ||
can be provided, each Task will validate that the amount of objects provided | ||
is suitable for its requirement. | ||
task : Task |
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.
You said tasks work as evaluators, but here I see I can only pass one task.
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.
Yeah I consciously constrained the power of Prism, task as we imagined them would be of 2 types:
- Calculations on Lens results, produce numerical results (compare for example)
- Operations on the instances, for example creating new ones based on sampling of datasets
I imagine that at the end of a Prism run some form of numerical results need to be produced.
That being the case, a Type 2 task would require another task of type 1 to be run after, so that numerical output can be produced. This would require to enforce order of execution (or to add task dependency on other tasks).
I decided to avoid the complication by allowing for one task to call another one. So for example an hypothetical cross-validation task would:
- Create the Lens instances by splitting dataset appropriately
- Run
compare
to produce numerical results
ATM I don't imagine the need to run super complex sequences of tasks, this might be a reflection on the fact that we don't have an actual use case for Prism yet.
from typing import Callable, List, Literal, Optional | ||
|
||
import numpy as np | ||
from pandas import DataFrame, concat |
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.
My preference for import package as
is because I find it more explicit. We import the entire package regardless and you can easily see that this is "pd.DataFrame", rather than some other DataFrame. It's also the convention I see everywhere with pandas and numpy.
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.
As mentioned, I really don't think this ID implementation is great. Or, at least, the ID specification conflicts with other "sources of truth" about artifacts reflected in get_container_info
. I also don't think parsing an ID string is reasonable - why not query the evaluator directly during the compare step?
All that said, I don't think we should invest more time in this. It works, let's move on.
Prototype for Prism object
Overall folder/file structure
comparators: Contains the basic logic to compare different type of results. Comparators can be a function of the result container type, and potentially the evaluators
compare.py: this is a type of task that prism can orchestrate. ATM it's only calling metric_comparator, in the future
compare.Compare
will instantiate the necessary comparators (table, metric, etc...) depending on the existing results containers.prism.py: the orchestrator, this is the equivalent of Lens but at a higher level. The purpose of Prism is to coordinate the various tasks. Currently when the user instantiates it, they can pass a dictionary of tasks to perform on the Lens objects. The dictionary is in the format
{'task_name:{'parameter':value}}
, where parameters are specific to the type of task to perform.Missing
compare.Compare
). I haven't created an abstract class for this yet, I am wondering if there is a need for it, or if the variety of the object will be too big for an abstract class to be useful.To test the run you can use the following code: