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

Refactor 836 refactor convert inputs and outputs to class property #837

Conversation

davidberenstein1957
Copy link
Member

@davidberenstein1957 davidberenstein1957 commented Jul 29, 2024

Closes #836
Closes #839

Comment on lines -212 to -236
def model_post_init(self, __context: Any) -> None:
"""Checks that the provided `LLM` uses the `MagpieChatTemplateMixin`."""
super().model_post_init(__context)

if not isinstance(self.llm, MagpieChatTemplateMixin):
raise ValueError(
f"`Magpie` task can only be used with an `LLM` that uses the `MagpieChatTemplateMixin`."
f"`{self.llm.__class__.__name__}` doesn't use the aforementioned mixin."
)

self.llm.use_magpie_template = True
Copy link
Member Author

Choose a reason for hiding this comment

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

@gabrielmbmb was there a reason you were defining these again? They were also defined in the parent class.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, having the model_post_init in the MagpieBase class and then inheriting in Magpie and MagpieGenerator didn't trigger it, so that's why it's duplicate in Magpie and MagpieGenerator tasks. Apparently, this is an issue with pydantic that should be fixed but it's not working and didn't want to expend too much time so I decided to duplicate the code.

Copy link

github-actions bot commented Aug 6, 2024

Documentation for this PR has been built. You can view it at: https://distilabel.argilla.io/pr-837/

Base automatically changed from develop to main August 6, 2024 12:25
Copy link

codspeed-hq bot commented Aug 6, 2024

CodSpeed Performance Report

Merging #837 will not alter performance

Comparing feat/836-refactor-convert-inputs-and-outputs-to-class-property (55f0183) with develop (c8df5a9)

Summary

✅ 1 untouched benchmarks

@davidberenstein1957 davidberenstein1957 linked an issue Aug 6, 2024 that may be closed by this pull request
@davidberenstein1957 davidberenstein1957 marked this pull request as ready for review August 6, 2024 16:35
@davidberenstein1957 davidberenstein1957 changed the title refactor: 836 refactor convert inputs and outputs to class property [WIP] Refactor 836 refactor convert inputs and outputs to class property Aug 6, 2024
@davidberenstein1957
Copy link
Member Author

davidberenstein1957 commented Aug 6, 2024

@gabrielmbmb there is one failing test that run locally but seems to fail in the test pipeline and I cannot pinpoint directly why. Do you have any idea?

For some reason, the dataset loaded by the file system does not have any columns.

Additionally, I am not 100% if the pipeline validation still works because I remove inputs and outputs from the pipeline integration tests and the pipelines don't raise any errors on run.

@gabrielmbmb gabrielmbmb changed the base branch from main to develop August 14, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants