Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

CI test failures after BB2 merge #3823

Merged
merged 20 commits into from
Jul 22, 2021
Merged

CI test failures after BB2 merge #3823

merged 20 commits into from
Jul 22, 2021

Conversation

mojtaba-komeili
Copy link
Contributor

Patch description
Resolving the issues with the failing unit tests on CircleCI that were introduced after merging BB2 and its dependent projects: Personal Knowledge, and Wizard of Internet.

@klshuster
Copy link
Contributor

klshuster commented Jul 20, 2021

are we prepared to require torch 1.8 or higher?

@mojtaba-komeili
Copy link
Contributor Author

are we prepared to require torch 1.8 or higher?

Probably not, I was just checking with another version because of Roller's note about the version 1.7 version.

requirements.txt Outdated
@@ -50,4 +49,4 @@ Unidecode==1.1.1
urllib3>=1.26.5
websocket-client==0.56.0
websocket-server==0.4
jsonlines==1.2.0
jsonlines==1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put this back in alphabetical order?

Copy link
Contributor

Choose a reason for hiding this comment

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

Pseudo-alphabetical order lol. there are some intentional non-monotonic places

Copy link
Contributor

Choose a reason for hiding this comment

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

ok yeah i think we might've broken some stuff with the order wrong... sorry @mojtaba-komeili could you just revert to what it was before?



@testing_utils.skipUnlessGPU
@unittest.skipIf(LOCAL, "Skipping Test because its slow and mem intensive")
@unittest.skipUnless(TRANSFORMER_INSTALLED, "Needs transformer, not installed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit funny because our GPU tests should always have transformers?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah im positive this should not be necessary

@stephenroller
Copy link
Contributor

The policy is usually past two versions of pytorch officially... So yeah requiring 1.8+ is viable now, but afaik, there's nothing we have that's 1.7 incompatible... If we're going to do that, I'd suggest a different PR to carve out 1.8/1.9 tests, and then rebase this

Comment on lines +27 to +63
if TRANSFORMER_INSTALLED:
SEARCH_QUERY_MODEL = ZOO_MEMORY_DECODER
PERSONA_SUMMARY_MODEL = ZOO_QUERY_GENERATOR
ZOO_BB2 = 'zoo:blenderbot2/blenderbot2_400M/model'
ZOO_BB2_3B = 'zoo:blenderbot2/blenderbot2_3B/model'
SEARCH_SERVER = '<SERVER_API>'
common_opt = {
'model': 'projects.blenderbot2.agents.blenderbot2:BlenderBot2RagAgent',
# rag args
'init_opt': 'arch/bart_large',
'generation_model': 'bart',
'retriever_debug_index': 'compressed',
'label_truncate': 128,
'text_truncate': 512,
'batchsize': 4,
'fp16': True,
'model_parallel': True,
# train args
'task': 'convai2,wizard_of_wikipedia',
'num_examples': 8,
}

def _test_bb2_rag(retrieval_method: KnowledgeAccessMethod, **kwargs):
opt = copy.deepcopy(common_opt)
opt['knowledge_access_method'] = retrieval_method.value
opt.update(dict(kwargs))
print(' '.join([f'--{k} {v}' for k, v in opt.items()]))
testing_utils.eval_model(opt, skip_test=True)
torch.cuda.empty_cache()

def _test_bb2_fid(retrieval_method: KnowledgeAccessMethod, **kwargs):
opt = copy.deepcopy(common_opt)
opt['model'] = 'projects.blenderbot2.agents.blenderbot2:BlenderBot2FidAgent'
opt['knowledge_access_method'] = retrieval_method.value
opt.update(dict(kwargs))
testing_utils.eval_model(opt, skip_test=True)
torch.cuda.empty_cache()
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this protected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because if we can't import projects.blenderbot2 we don't have constants such as ZOO_MEMORY_DECODER so we need to skip everything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually does CircleCI even run tests in nightly/gpu if it is not running the GPU tests (that have transformer)? I tried to debug running local pytest with transformer not installed, and they failed. But now thinking maybe it doesn't work like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

this marker runs the nightly gpu tests: https://github.com/facebookresearch/ParlAI/blob/master/.circleci/config.yml#L388

you can see that the deps under torchgpu1.7 are installed, which includes transformers: https://github.com/facebookresearch/ParlAI/blob/master/.circleci/config.yml#L95



@testing_utils.skipUnlessGPU
@unittest.skipIf(LOCAL, "Skipping Test because its slow and mem intensive")
@unittest.skipUnless(TRANSFORMER_INSTALLED, "Needs transformer, not installed.")
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah im positive this should not be necessary

@klshuster klshuster merged commit dd16d3f into master Jul 22, 2021
@klshuster klshuster deleted the bb2-testfails branch July 22, 2021 18:24
@klshuster klshuster mentioned this pull request Jul 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants