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

Hlu/update entailment notebook running time #237

Merged
merged 6 commits into from Aug 7, 2019

Conversation

hlums
Copy link
Collaborator

@hlums hlums commented Aug 2, 2019

Description

Add a QUICK_RUN flag and reference running time to the entailment notebook.
Add integration test to the entailment notebook.

If this looks good, I will do the same thing to most of the other notebooks. May not be able to do it on the QnA notebook and sentence similarity notebook because it may be hard to subset the data in those notebooks. Need to take a closer look to decide.

Related Issues

#213

Checklist:

  • My code follows the code style of this project, as detailed in our contribution guidelines.
  • I have added tests.
  • I have updated the documentation accordingly.

@review-notebook-app
Copy link

Check out this pull request on ReviewNB: https://app.reviewnb.com/microsoft/nlp/pull/237

You'll be able to see notebook diffs and discuss changes. Powered by ReviewNB.

Copy link
Member

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

this is really good

@miguelgfierro
Copy link
Member

hey @hlums there is a small conflict in the conftest.

One question, are you sure this piece of code is ok:

@pytest.fixture(scope="module")
def teardown_service(
    subscription_id, resource_group, workspace_name, workspace_region
):

    yield

    # connect to workspace
    ws = azureml_utils.get_or_create_workspace(
        config_path="tests/ci",
        subscription_id=subscription_id,
        resource_group=resource_group,
        workspace_name=workspace_name,
        workspace_region=workspace_region,
    )

    # connect to aci_service
    aci_service = Webservice(workspace=ws, name="aci-test-service")

    # delete aci_service
    aci_service.delete()

What does this yeild does?

@hlums
Copy link
Collaborator Author

hlums commented Aug 5, 2019

hey @hlums there is a small conflict in the conftest.

One question, are you sure this piece of code is ok:

@pytest.fixture(scope="module")
def teardown_service(
    subscription_id, resource_group, workspace_name, workspace_region
):

    yield

    # connect to workspace
    ws = azureml_utils.get_or_create_workspace(
        config_path="tests/ci",
        subscription_id=subscription_id,
        resource_group=resource_group,
        workspace_name=workspace_name,
        workspace_region=workspace_region,
    )

    # connect to aci_service
    aci_service = Webservice(workspace=ws, name="aci-test-service")

    # delete aci_service
    aci_service.delete()

What does this yeild does?

@miguelgfierro I actually don't know. I think it showed up in my PR because of black autoformatting. Looking at the history, @cocochrane added this. @cocochrane, can you explain this?

@cocochrane
Copy link
Contributor

I researched how to write fixtures and the yield statement essentially allows the test to run before the rest of the fixture proceeds But yes, I don't know why this is showing up in her PR because this was already merged into staging and works as expected

@hlums
Copy link
Collaborator Author

hlums commented Aug 5, 2019

I researched how to write fixtures and the yield statement essentially allows the test to run before the rest of the fixture proceeds But yes, I don't know why this is showing up in her PR because this was already merged into staging and works as expected

Thanks! It showed up because of black auto-formatting. :)



@pytest.mark.integration
def test_entailment_multinli_bert(notebooks):
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add @pytest.mark.gpu so this is executed with the gpu tests. Is there a reason why we also wanted to check this notebook on cpu?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. One of the reasons is that I'm still getting familiar with the pytest marks. :)
Previously, we did have a problem that the notebook works on GPU but not on CPU, but It's now fixed and the test is covered by the unit tests.

@miguelgfierro miguelgfierro merged commit a4e2e28 into staging Aug 7, 2019
@miguelgfierro miguelgfierro deleted the hlu/update_entailment_notebook_running_time branch August 7, 2019 10:32
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.

None yet

3 participants