Skip to content

Conversation

@bdura
Copy link
Contributor

@bdura bdura commented May 23, 2023

Description

This PR adds a new REL task.

Types of change

This is a feature.

Checklist

  • I confirm that I have the right to submit this contribution under the project's MIT license.
  • I ran all tests , and all new and existing tests passed. This includes tests marked as external and those
    requiring a GPU. I did this for the following directories:
    • tests
    • usage_example/tests
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

@bdura bdura marked this pull request as draft May 23, 2023 12:19
@rmitsch rmitsch added the feat/task Feature: tasks label May 23, 2023
@bdura bdura marked this pull request as ready for review May 23, 2023 15:37
Copy link
Collaborator

@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.

Very nice work!

rmitsch added 2 commits May 24, 2023 08:45
# Conflicts:
#	usage_examples/tests/test_usage_examples.py
@rmitsch
Copy link
Collaborator

rmitsch commented May 24, 2023

I adjusted the usage examples (and usage example tests) style to be in line with the other ones. I noticed that there is a Thinc deprecation warning happening in the tests however - let me see if I can reproduce it in the GH CI.

@rmitsch
Copy link
Collaborator

rmitsch commented May 24, 2023

Ah, and first we need to add spacy download en_core_web_md in the CI pipeline (see failing external tests).

@rmitsch
Copy link
Collaborator

rmitsch commented May 24, 2023

@bdura This is the warning I mean. Can you reproduce this locally?

otherwise multiprocessing won't work
@bdura
Copy link
Contributor Author

bdura commented May 24, 2023

Ok so for now let's leave it as it is? It requires quite an important change to use spacy assemble (or a custom utility), and I think it's better handled with all examples at once, in another PR?

@adrianeboyd
Copy link
Contributor

Add [initialize.vectors] the configs that source from en_core_web_md and then add nlp.initialize() to the scripts that run the examples.

@adrianeboyd
Copy link
Contributor

Ah, wait, no that won't quite work, either. Let me just write a util method...

@bdura
Copy link
Contributor Author

bdura commented May 24, 2023

Great, thanks for the help @adrianeboyd!

I think it's finally ready.

Co-authored-by: Adriane Boyd <[email protected]>
@rmitsch
Copy link
Collaborator

rmitsch commented May 25, 2023

@bdura Before you merge, run the external tests again? (Locally or GH, doesn't matter)

@bdura
Copy link
Contributor Author

bdura commented May 25, 2023

@bdura Before you merge, run the external tests again? (Locally or GH, doesn't matter)

Still some failures. Thanks, I'll investigate.

@bdura bdura merged commit e20984b into main May 25, 2023
@rmitsch rmitsch deleted the feat/rel branch May 26, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat/task Feature: tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants