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

set use_cdn=False in notebook tests #281

Merged

Conversation

imatiach-msft
Copy link
Collaborator

set use_cdn=False in notebook tests

# Don't use CDN when running notebook tests
for cell in notebook['cells']:
if cell.cell_type == 'code' and "ExplanationDashboard(" in cell.source:
if "ExplanationDashboard(global_explanation, model, datasetX=x_test)" in cell.source:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems a brittle way of doing this. Would be easier to just have an alternate cell run, but at that point, can we just have a notebook configured for automated testing? I'm not seeing how overwriting a cell in the test is more agreeable than having separate notebooks for testing. Also, I think like 25 should be False, not True.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't want to have separate notebook, we want to keep using the same notebooks. We don't want to have duplicate copies of notebooks in the repo, where one is just for testing, that would make updating notebooks difficult. We already modify the notebook run in tests to add additional cells with validation logic.

"Also, I think like 25 should be False, not True."
Good catch, will update!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, updated!

@imatiach-msft imatiach-msft merged commit a6b72e5 into interpretml:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants