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

Fix spacy_stanza.md example #1235

Merged
merged 3 commits into from
Dec 26, 2023
Merged

Fix spacy_stanza.md example #1235

merged 3 commits into from
Dec 26, 2023

Conversation

ksjogo
Copy link
Contributor

@ksjogo ksjogo commented Dec 18, 2023

Change Description

The example was missing the super call in the constructor. Thus the example will fail as the super constructor initialized the ner config.

Issue reference

This PR fixes issue #XX

Checklist

  • I have reviewed the contribution guidelines
  • I have signed the CLA (if required)
  • My code includes unit tests
  • All unit tests and lint checks pass locally
  • My PR contains documentation updates / additions if required

@omri374
Copy link
Contributor

omri374 commented Dec 24, 2023

Hi, I'm not sure that passing the loaded spacy model into the parent class init is the right way to go.
Looking at the init for SpacyNlpEngine, it accepts a models dict and an ner_model_configuration

ner_model_configuration: Optional[NerModelConfiguration] = None,

The models are already loaded, so no need to pass them. If a user requires a different NerModelConfiguration, then this should be passed to the super.

@omri374
Copy link
Contributor

omri374 commented Dec 26, 2023

Closing, please re-open if you'd like to continue working on this.

@omri374 omri374 closed this Dec 26, 2023
@ksjogo
Copy link
Contributor Author

ksjogo commented Dec 26, 2023

@omri374 don't have rights to re-open PRs in this repo.
Did remove the argument in my origin branch. But I think it is not updating closed MRs.

@omri374 omri374 reopened this Dec 26, 2023
@omri374
Copy link
Contributor

omri374 commented Dec 26, 2023

Reopened

Copy link
Contributor

@omri374 omri374 left a comment

Choose a reason for hiding this comment

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

Thanks!

@omri374 omri374 merged commit a3facc8 into microsoft:main Dec 26, 2023
2 checks passed
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