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

PLbart support #709

Merged
merged 30 commits into from
Jul 13, 2024
Merged

PLbart support #709

merged 30 commits into from
Jul 13, 2024

Conversation

FahadEbrahim
Copy link
Contributor

Hi,

This is a PR to add the PLBART model written in the following paper. The original code is available at the following link. It follows the BART architecture, so, I followed the same style.

This is my first PR, so apologies in advance for any errors and I'd appreciate your feedback and guidance.

Thanks,
Fahad.

@calpt
Copy link
Member

calpt commented Jun 24, 2024

Hey @FahadEbrahim,

Thank you so much for working on this! We'll look into your PR more closely in the coming days. In the meantime, it would be great if you could continue to work on making the failing checks passing and let us know if you need any help on that!

@FahadEbrahim
Copy link
Contributor Author

FahadEbrahim commented Jun 25, 2024

Thank you @calpt for your kind reply.

The testing of the models and the methods were successful.
But I'd appreciate your review conceptually (if there are differences between PLBart and BART that I'm unaware of)

I was having an issue with the first test of code quality. It was due to not cloning the repo recursively.
It's been fixed now.

Copy link
Member

@calpt calpt left a comment

Choose a reason for hiding this comment

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

Finished a review pass of the changes in this PR and everything looks good to me (just one minor comment)! Would be awesome to see any adapters you trained with this implementation on the hub.

After comments & merge conflicts are resolved, we're good to merge from our side. Thanks again for working on this!

src/adapters/head_utils.py Outdated Show resolved Hide resolved
@FahadEbrahim
Copy link
Contributor Author

Thank you @calpt for reviewing the PR. The conflict has been fixed.
Please let me know if anything can be modified.

@calpt calpt merged commit a6387c1 into adapter-hub:main Jul 13, 2024
4 checks passed
@FahadEbrahim FahadEbrahim deleted the plbart_support branch July 15, 2024 17:25
dainis-boumber added a commit to ReDASers/adapters that referenced this pull request Aug 26, 2024
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