Ouranosinc / pavics-sdi

Power Analytics and Visualization for Climate Science - Spatial Data Infrastructure
https://pavics-sdi.readthedocs.io
6 stars 2 forks source link

Ensure that notebooks build on ReadTheDocs #307

Closed Zeitsperre closed 8 months ago

review-notebook-app[bot] commented 12 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Zeitsperre commented 12 months ago

This is really bizarre. We're failing because the recommonmark dependency is not installing properly, however, this shouldn't be added at all according to this: https://blog.readthedocs.com/python-core-requirements-changed/

Edit: Apparently this only goes into effect for new repos.

Zeitsperre commented 12 months ago

@huard

It seems like we're running into this issue when building the docs: https://github.com/executablebooks/MyST-NB/issues/464

I'll disable fail_on_warning for now, but we'll need to manually check that everything looks right from now on until changes happen upstream.

Zeitsperre commented 12 months ago

I think I'm done here for now.

The remaining warnings are due to some services now being offline (flyingpigeon) or because of issues with rendering hvplot/panel output in MyST (doesn't seem to be supported yet). That said, the notebooks are rendering/running much better now.

Zeitsperre commented 8 months ago

@tlvu

Think you could give this another look when you have time? Let's close merge this PR next week.

fmigneault commented 8 months ago

@tlvu @Zeitsperre

Since this PR was merged, all test builds fail for the Weaver notebook. Only the final cell checking the file contents from the result of ncdump in the job fails the output comparison (diff with missing extra new lines) :

18:00:00  _ pavics-sdi-master/docs/source/notebook-components/weaver_example.ipynb::Cell 9 _
18:00:00  Notebook cell execution failed
18:00:00  Cell 9: Cell outputs differ
18:00:00  
18:00:00  Input:
18:00:00  # If execution succeeded, the results endpoint will return 200 with corresponding references.
18:00:00  # Otherwise, 400 occurs because results were not produced due to failing job, and requesting its outputs is an invalid request.
18:00:00  path = f"{status_location}/results"
18:00:00  resp = requests.get(path, **WEAVER_TEST_REQUEST_XARGS)
18:00:00  assert (
18:00:00      resp.status_code == 200
18:00:00  ), f"Failed to retrieve job results location [{path}]. Code: [{resp.status_code}]."
18:00:00  print("\nJob was successful! Retrieving result location...")
18:00:00  body = resp.json()
18:00:00  
18:00:00  # Here, our target output ID is named 'output' according to the process description
18:00:00  output = body.get("output")
18:00:00  assert isinstance(
18:00:00      output, dict
18:00:00  ), f"Could not find result matching ID 'output' within:\n{json_dump(body)}"
18:00:00  href = output["href"]
18:00:00  assert isinstance(href, str) and href.startswith(
18:00:00      WEAVER_TEST_WPS_OUTPUTS
18:00:00  ), f"Output result location does not have expected reference format: [{href}]"
18:00:00  print(f"Result is located at: [{href}]\n")
18:00:00  assert href.endswith(".txt")
18:00:00  
18:00:00  print("Fetching output contents...")
18:00:00  resp = requests.get(href)
18:00:00  print(f"\nNCDUMP 'output' result content:\n\n{resp.text}")
18:00:00  
18:00:00  
18:00:00  Traceback:
18:00:00   mismatch 'stdout'
18:00:00  
18:00:00   assert reference_output == test_output failed:
18:00:00  
18:00:00    '\nJob was su..."time" ;\n}\n' == '\nJob was su...e" ;\n}\n\n\n'
18:00:00    Skipping 3445 identical leading characters in diff, use -v to show
18:00:00      "time" ;
18:00:00      }
18:00:00    - 
18:00:00    - 
tlvu commented 8 months ago

Since this PR was merged, all test builds fail for the Weaver notebook. Only the final cell checking the file contents from the result of ncdump in the job fails the output comparison (diff with missing extra new lines) :

Maybe you just need to refresh the output of the Weaver notebook, like I had to do in this PR https://github.com/Ouranosinc/pavics-sdi/pull/316 ?

tlvu commented 8 months ago

Since this PR was merged, all test builds fail for the Weaver notebook. Only the final cell checking the file contents from the result of ncdump in the job fails the output comparison (diff with missing extra new lines) :

Maybe you just need to refresh the output of the Weaver notebook, like I had to do in this PR #316 ?

Forgot to say that I did not notice Weaver notebook was failing because our overnight Jenkins run against production host, it uses the defaults settings in Jenkins only, which Weaver is not enabled.

fmigneault commented 8 months ago

@tlvu

Maybe you just need to refresh the output of the Weaver notebook

I agree, but was wondering whether these changes were introduced by pre-commit. I don't see why the weaver notebook would have been modified only for the last extra newlines otherwise. If pre-commit is indeed the one that cause the changes, they would be reverted immediately after "unfixing" them.

tlvu commented 8 months ago

I agree, but was wondering whether these changes were introduced by pre-commit. I don't see why the weaver notebook would have been modified only for the last extra newlines otherwise. If pre-commit is indeed the one that cause the changes, they would be reverted immediately after "unfixing" them.

I don't think it's the pre-commit. The pre-commit runs on every single PR. So if it does something wrong, it would also affect the PR that "fixes" it by unfixing it again.

I think it's the version of xarray or other packages that changes in the Jupyter env. Different version seems to generate different output format.

fmigneault commented 8 months ago

Ok. I'll give it a try and see what happens.