refactor: update required resources treatment and use subclasses over mixins #184
+114
−175
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnnygreco
commented
Jan 7, 2026
johnnygreco
commented
Jan 7, 2026
|
|
||
|
|
||
| class WithChatCompletionGeneration(WithModelGeneration): | ||
| class ColumnGeneratorWithSingleModelChatCompletion(ColumnGeneratorWithSingleModel[TaskConfigT]): |
Contributor
Author
There was a problem hiding this comment.
This class name is getting a bit long for my liking lol
Contributor
There was a problem hiding this comment.
may be we drop single from these names assuming we mostly work with one model by default
johnnygreco
changed the title
johnnygreco
changed the title
johnnygreco
force-pushed
the
johnny/refactor/remove-required-resources-from-metadata
branch
from
d7d6e9c to
c4f717b
Compare
nabinchha
reviewed
Jan 8, 2026
| logger.info(f" |-- column name: {self.config.name!r}") | ||
| logger.info(f" |-- model config:\n{self.model_config.model_dump_json(indent=4)}") | ||
| if self.model_config.provider is None: | ||
| logger.info(f" |-- default model provider: {self._get_provider_name()!r}") |
Contributor
There was a problem hiding this comment.
We're loosing this log message, which was added at some point to indicate the default mode provider being used when model config itself doesn't reference a provider.
Contributor
Author
There was a problem hiding this comment.
The update makes it so this message is always logged, right? At least that was my intention. WDYT?
|
|
||
|
|
||
| class WithChatCompletionGeneration(WithModelGeneration): | ||
| class ColumnGeneratorWithSingleModelChatCompletion(ColumnGeneratorWithSingleModel[TaskConfigT]): |
Contributor
There was a problem hiding this comment.
may be we drop single from these names assuming we mostly work with one model by default
| ) | ||
|
|
||
| def _fan_out_with_threads(self, generator: WithModelGeneration, max_workers: int) -> None: | ||
| def _fan_out_with_threads(self, generator: ColumnGeneratorWithSingleModel, max_workers: int) -> None: |
Contributor
There was a problem hiding this comment.
should this be at the ColumnGeneratorWithModelRegistry level?
johnnygreco
force-pushed
the
johnny/refactor/remove-required-resources-from-metadata
branch
from
c4f717b to
1b18b73
Compare
johnnygreco
commented
Jan 9, 2026
Comment on lines
-65
to
-99
| def column_type_used_in_execution_dag(column_type: str | DataDesignerColumnType) -> bool: | ||
| """Return True if the column type is used in the workflow execution DAG.""" | ||
| column_type = resolve_string_enum(column_type, DataDesignerColumnType) | ||
| dag_column_types = { | ||
| DataDesignerColumnType.EXPRESSION, | ||
| DataDesignerColumnType.LLM_CODE, | ||
| DataDesignerColumnType.LLM_JUDGE, | ||
| DataDesignerColumnType.LLM_STRUCTURED, | ||
| DataDesignerColumnType.LLM_TEXT, | ||
| DataDesignerColumnType.VALIDATION, | ||
| DataDesignerColumnType.EMBEDDING, | ||
| } | ||
| dag_column_types.update(plugin_manager.get_plugin_column_types(DataDesignerColumnType)) | ||
| return column_type in dag_column_types | ||
|
|
||
|
|
||
| def column_type_is_model_generated(column_type: str | DataDesignerColumnType) -> bool: | ||
| """Return True if the column type is a model-generated column.""" | ||
| column_type = resolve_string_enum(column_type, DataDesignerColumnType) | ||
| model_generated_column_types = { | ||
| DataDesignerColumnType.LLM_TEXT, | ||
| DataDesignerColumnType.LLM_CODE, | ||
| DataDesignerColumnType.LLM_STRUCTURED, | ||
| DataDesignerColumnType.LLM_JUDGE, | ||
| DataDesignerColumnType.EMBEDDING, | ||
| } | ||
| model_generated_column_types.update( | ||
| plugin_manager.get_plugin_column_types( | ||
| DataDesignerColumnType, | ||
| required_resources=["model_registry"], | ||
| ) | ||
| ) | ||
| return column_type in model_generated_column_types | ||
|
|
||
|
|
Contributor
Author
There was a problem hiding this comment.
Moved these both to engine
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem we are solving
ConfigurableTaskMetadatarequires you to specify the resources required to execute the task. In practice, however, all tasks always have access to all resources. While this issue isn’t a big deal, it is confusing for plugin builders who don’t have the above context.Changes
Remove
required_resourcesfrom metadataAlways assume all tasks have access to all resources.
Use subclasses (instead of mixins) to streamline development and simplify plugin development.
An important note is that I think we still need some what for a generator to specify its required resources. For example, say we want to filter plugin types to only grab the ones that need the model registry or just need the datastore. The solution implemented here is an abstract method called
get_required_resourcesthat must be implemented on generators. This effectively pushes this complication to a lower lever, where most developers won't need to worry about it. I'll highlight some places in the code to show what I mean.