Skip to content

Conversation

@ahmeshaf
Copy link

@ahmeshaf ahmeshaf commented Jul 21, 2023

Span-based Semantic Role Labeling (SRL) with spacy-llms

Description

This PR is for the addition of a fully working spacy pipeline for zero-shot span-based SRL using OpenAI. It includes the task, template, usage example, tests (pytest), and documentation.

Types of change

new feature

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran all tests in tests and usage_examples/tests, and all new and existing tests passed. This includes
    • all external tests (i. e. pytest ran with --external)
    • all tests requiring a GPU (i. e. pytest ran with --gpu)
  • My changes require a change to the documentation, and I've added all the required information.

@rmitsch
Copy link
Contributor

rmitsch commented Jul 26, 2023

@ahmeshaf Is this ready to be reviewed?

@ahmeshaf
Copy link
Author

@rmitsch Yes, it is!

Copy link
Contributor

@rmitsch rmitsch left a comment

Choose a reason for hiding this comment

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

Will be great to have this, thank you!

reads a file containing task examples for few-shot learning. If None is
passed, then zero-shot learning will be used.
normalizer (Optional[Callable[[str], str]]): optional normalizer function.
alignment_mode (str): "strict", "contract" or "expand".
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
alignment_mode (str): "strict", "contract" or "expand".
alignment_mode (Literal["strict", "contract", "expand"]): "strict", "contract" or "expand".

And the description should refer to what it does (I know this is not on you, we have that in other places already unfortunately).

Copy link
Author

Choose a reason for hiding this comment

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

Where can I find the descriptions for these? will copy-paste

Copy link
Contributor

Choose a reason for hiding this comment

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

E. g. here.

Copy link
Author

Choose a reason for hiding this comment

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

done. be50655

@rmitsch
Copy link
Contributor

rmitsch commented Jul 27, 2023

The spacy.io docs also need to be updated (see here).

@svlandeg svlandeg added feat/task Feature: tasks enhancement Improvement of existing feature labels Aug 2, 2023
@rmitsch
Copy link
Contributor

rmitsch commented Aug 7, 2023

@ahmeshaf Are you interested in continuing working on this? Otherwise I can take it over, we'd definitely want to have this in spacy-llm :)

@ahmeshaf
Copy link
Author

ahmeshaf commented Aug 7, 2023

@ahmeshaf Are you interested in continuing working on this? Otherwise I can take it over, we'd definitely want to have this in spacy-llm :)

Will work on it this week!

@rmitsch
Copy link
Contributor

rmitsch commented Aug 11, 2023

@ahmeshaf Let me know when this is ready for another review!

@ahmeshaf
Copy link
Author

@rmitsch it is ready! although there are a few unresolved comments above where I could use some help :)

@rmitsch
Copy link
Contributor

rmitsch commented Aug 16, 2023

@ahmeshaf Responded to the open threads. Let me know when you're done with incorporating this - then I'll do (hopefully the last) review round 🙂

@ahmeshaf
Copy link
Author

@rmitsch this is ready for a review!

@rmitsch
Copy link
Contributor

rmitsch commented Sep 7, 2023

Thanks, @ahmeshaf! Should get around to it Friday or next week 🤞

rmitsch
rmitsch previously approved these changes Sep 18, 2023
Copy link
Contributor

@rmitsch rmitsch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🎉 We refactored the task structure a while ago and moved the docs to spacy.io, but that doesn't matter for this PR - we'll adjust the code and do separate PRs for the refactor and docs. I'll merge this into a new branch until we have the time to finish this up in develop.

[Edit] Since the refactor is already in main, the approach before isn't viable. Instead I'll leave this open and refactor this on your branch, @ahmeshaf.

@rmitsch rmitsch added Test external Run external tests Test GPU Run GPU tests labels Sep 18, 2023
@rmitsch rmitsch dismissed their stale review September 18, 2023 14:30

Has to be refactored before merging.

@rmitsch rmitsch changed the base branch from main to develop September 21, 2023 12:47
@rmitsch rmitsch changed the base branch from develop to main September 21, 2023 12:48
@rmitsch
Copy link
Contributor

rmitsch commented Sep 21, 2023

Closing in favor of #301.

@rmitsch rmitsch closed this Sep 21, 2023
@ahmeshaf
Copy link
Author

Oh. Thanks @rmitsch! Maybe, I could give the refactoring a shot if it saves your time?

@rmitsch
Copy link
Contributor

rmitsch commented Sep 22, 2023

@ahmeshaf Let's continue the conversation in #301 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement of existing feature feat/task Feature: tasks Test external Run external tests Test GPU Run GPU tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants