Skip to content
This repository has been archived by the owner on Dec 16, 2022. It is now read-only.

Find the right embedding layer for mismatched cases #4179

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

matt-gardner
Copy link
Contributor

I was diagnosing an error in the interpret code for a scibert NER model, and found this. Comments in the code should explain the problem and solution well enough, let me know if they don't.

@matt-gardner
Copy link
Contributor Author

The model test failure here is actually a bug in the test that I need to fix. It's a one-line change in the model repo. What's the process for doing this, given that we're running model tests here, and I can't add a fix to this PR?

@epwalsh
Copy link
Member

epwalsh commented Apr 30, 2020

@matt-gardner just make a separate PR in the models repo. We can merge that into master and then re-run this workflow

@matt-gardner
Copy link
Contributor Author

But that PR will fail tests without this change.

@epwalsh
Copy link
Member

epwalsh commented Apr 30, 2020

You could temporarily update the workflow in models CI to use your patch branch by modifying the requirements.txt file

@matt-gardner
Copy link
Contributor Author

That seems pretty bad - I wouldn't want to merge such a PR into that repo. I can see modifying the allennlp commit in the models repo to demonstrate that we have two PRs, one on each repo, that both work, before either one gets merged. But I think we're going to have to live with the fact the we'll have a short window where one or the other CI is broken. I'm not sure there's a reasonable alternative. @dirkgr, do you see a way around this, either?

@epwalsh
Copy link
Member

epwalsh commented Apr 30, 2020

Personally I'm okay with one repo being broken for a few minutes

@matt-gardner
Copy link
Contributor Author

Me too. I'll make another PR for the model repo, to show that we have a fix for the breakage in this PR, then after this PR gets merged I can update the other PR to remove the CI hack, and merge the fix for the model repo tests.

@matt-gardner
Copy link
Contributor Author

Ok, corresponding model PR: allenai/allennlp-models#38.

@matt-gardner matt-gardner merged commit 2544e59 into allenai:master Apr 30, 2020
@matt-gardner matt-gardner deleted the mismatched_interpret branch April 30, 2020 22:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants