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

chore: Add prompt eval to task type #629

Merged
merged 3 commits into from
May 25, 2023
Merged

chore: Add prompt eval to task type #629

merged 3 commits into from
May 25, 2023

Conversation

setu4993
Copy link
Member

Just adding the new task type to keep dataquality aligned.

@setu4993 setu4993 requested a review from elboy3 May 23, 2023 16:49
@setu4993 setu4993 requested a review from a team as a code owner May 23, 2023 16:49
@setu4993 setu4993 force-pushed the chore/add-task-type branch 2 times, most recently from dcbfac4 to 73a0098 Compare May 23, 2023 19:36

@staticmethod
def get_valid_tasks() -> List[str]:
return list(map(lambda x: x.value, TaskType))
def get_valid_tasks() -> List["TaskType"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

let's keep this returning a string

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about it but it doesn't have any downstream impact, which is why I felt comfortable changing it. Curious why you think we should match the signature?

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh cool, i didn't know the enum in an f string would work, but since it does, i'm cool with it

def get_valid_tasks() -> List[str]:
return list(map(lambda x: x.value, TaskType))
def get_valid_tasks() -> List["TaskType"]:
"""Tasks that are valid for dataquality."""
Copy link
Contributor

Choose a reason for hiding this comment

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

if we do it this way, let's add more to this docstring to explain the context of what "valid" means and why prompt evaluation isn't "valid"

return [
task_type
for task_type in TaskType
if task_type not in [TaskType.prompt_evaluation]
Copy link
Contributor

Choose a reason for hiding this comment

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

i would maybe suggest adding a new static method something like get_logger_task_types or get_dq_task_types or something along those lines that excludes prompt eval instead of hijacking this fn

Copy link
Contributor

@elboy3 elboy3 left a comment

Choose a reason for hiding this comment

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

left a few thoughts

@setu4993 setu4993 requested a review from elboy3 May 24, 2023 08:07
@setu4993 setu4993 force-pushed the chore/add-task-type branch from 73a0098 to 388361d Compare May 24, 2023 20:10
@setu4993 setu4993 force-pushed the chore/add-task-type branch from 388361d to 053824e Compare May 24, 2023 20:27
@setu4993 setu4993 merged commit 38db557 into main May 25, 2023
@setu4993 setu4993 deleted the chore/add-task-type branch May 25, 2023 16:17
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