Add Fenced Docstring Testing #640
Conversation
| @@ -77,7 +77,7 @@ class BertBackbone(Backbone): | |||
| } | |||
|
|
|||
| # Pretrained BERT encoder | |||
| model = keras_nlp.models.BertBackbone.from_preset("base_base_en_uncased") | |||
| model = keras_nlp.models.BertBackbone.from_preset("bert_base_en_uncased") | |||
There was a problem hiding this comment.
lol, good we are running this :)
| MESSAGE = textwrap.dedent( | ||
| """\n | ||
| ############################################################## | ||
| # Check the documentation (go/g3doctest) on how to write |
There was a problem hiding this comment.
let's not include this link, it's not public
| ) | ||
| def test_fenced_docstrings(): | ||
|
|
||
| regex_pattern = re.compile( |
There was a problem hiding this comment.
Is this a pattern to skip? If so we should probably name/comment this to make it clearer.
I wonder if there is an annotation we could add that would make this more flexible (and work for both types of docstrings). @do_not_test_docstring or something like that.
| @@ -77,3 +94,59 @@ def test_docstrings(): | |||
| if not result.wasSuccessful(): | |||
| print(result) | |||
| assert result.wasSuccessful() | |||
|
|
|||
|
|
|||
| @pytest.mark.skipif( | |||
There was a problem hiding this comment.
we can probably remove this case everywhere, we aren't supporting win32 natively anymore (only through WSL).
| @pytest.mark.skipif( | ||
| sys.platform == "win32", reason="Numpy prints differently on windows" | ||
| ) | ||
| def test_fenced_docstrings(): |
There was a problem hiding this comment.
we should probably mark this a large test, this will involve a lot of file downloads for the preset right?
|
|
||
| suite.addTest( | ||
| doctest.DocFileSuite( | ||
| *keras_nlp_files, |
There was a problem hiding this comment.
I think you said you were going to look into doing this via modules and not files, is that still possible? What was the outcome of checking this out?
| from keras_nlp.tests.doc_tests.docstring_lib import DoctestOutputChecker | ||
|
|
||
|
|
||
| class FencedCellOutputChecker(DoctestOutputChecker): |
There was a problem hiding this comment.
we should attribute our source here
| textwrap.dedent( | ||
| """ | ||
| ********************************************************************* | ||
| * Caution: `fenced_doctest` patches `doctest.compile` don't use this |
There was a problem hiding this comment.
Is this a valid warning? If so, I'm pretty sure we will run both docstest in this same binary.
Maybe we are ok because this gets run second?
| So to let doctest act like a notebook: | ||
|
|
||
| 1. We need `mode="exec"` (easy) | ||
| 2. We need the last expression to be printed (harder). |
There was a problem hiding this comment.
I wonder if we want this... Nothing relies on output after a fenced code block today, but we could add this in.
Will discuss with folks.
|
@abheesht17 also it looks like things are failing on ci because of the lack of |
|
@chenmoneygithub, could you please generate a docker image for this for accelerator testing? I've added |
keras_nlp/models/backbone.py
Outdated
| @@ -49,12 +49,15 @@ def from_preset( | |||
| Defaults to `True`. | |||
|
|
|||
| Examples: | |||
| doctest.skip | |||
There was a problem hiding this comment.
Let's remember to remove this.
| ) | ||
|
|
||
| ASTOR_WARNING = ( | ||
| "`test_fenced_docstrings` requires `astor`. Please run `pip install astor`." |
There was a problem hiding this comment.
We don't usually keep warning string in constant like this, I'm just going to inline this.
Resolves #420
Copied over from https://github.com/tensorflow/tensorflow/blob/master/tensorflow/tools/docs/fenced_doctest_lib.py.
Two issues that have to be solved:
CCing @mattdangerw for suggestions/help!