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

Download single_pass_weld.wx directly in the tutorials #705

Merged
merged 10 commits into from
Feb 17, 2022

Conversation

vhirtham
Copy link
Collaborator

@vhirtham vhirtham commented Feb 15, 2022

Changes

I added a hidden cell at the top in each of the new tutorials. This cell calls the function to download the needed single_pass_weld.wx file. The background is that you can't run those tutorials locally without the file but it is only downloaded when in another directory if you actually build the docs. I also edit a flag to the download function to suppress text outputs.

Checks

  • updated CHANGELOG.rst
  • updated tests
  • updated doc/
  • update example/tutorial notebooks
  • update manifest file

@vhirtham vhirtham added documentation Improvements or additions to documentation no-changelog-entry-needed labels Feb 15, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@codecov
Copy link

codecov bot commented Feb 15, 2022

Codecov Report

Merging #705 (979b9b6) into master (8a2ccec) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #705   +/-   ##
=======================================
  Coverage   96.05%   96.05%           
=======================================
  Files          92       92           
  Lines        6393     6393           
=======================================
  Hits         6141     6141           
  Misses        252      252           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a2ccec...979b9b6. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Feb 15, 2022

Unit Test Results

       1 files  ±0         1 suites  ±0   1m 31s ⏱️ -28s
2 156 tests ±0  2 156 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 979b9b6. ± Comparison against base commit 8a2ccec.

♻️ This comment has been updated with latest results.

@vhirtham vhirtham requested a review from CagtayFabry February 15, 2022 12:27
@marscher
Copy link
Contributor

I think we could also remove the download inside the docs (conf.py), right?

@CagtayFabry
Copy link
Member

I think we could also remove the download inside the docs (conf.py), right?

wasn't the idea to download the file once for the tests so that it is available?

@CagtayFabry
Copy link
Member

I think it is cleaner to use the "nbsphinx": "hidden" cell metadata so that the download code doesn't show up in the html documentation

@marscher
Copy link
Contributor

If the notebooks download the file, we do not need to do it explicitly (even for the tests), right? That should be the case as all notebook cells will be executed by the tests in sequential order. The only thing that could happen then is a race condition, when two notebooks will start to download the file. This should be avoided as it leads to unpredictable test failures.
The current situation is, that we download during Sphinx building and only the first tutorial will download explicitly.
The download code has been duplicated to do so. I'd prefer a solution without duplication and having the notebooks download it seems to be the one.

@CagtayFabry
Copy link
Member

I just wasn't sure about the parallel execution of the notebooks via pytest but that's ok for me too if it works

@marscher
Copy link
Contributor

not downloading in Sphinx works. The RTD error is unrelated.
intersphinx inventory 'https://k3d-jupyter.org/objects.inv' not fetchable due to <class 'requests.exceptions.ConnectionError'>: HTTPSConnectionPool(host='k3d-jupyter.org', port=443): Max retries exceeded with url: /objects.inv (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7fe6e6f7c760>: Failed to establish a new connection: [Errno 110] Connection timed out'))

@vhirtham vhirtham merged commit 9f2bc65 into BAMWelDX:master Feb 17, 2022
@vhirtham vhirtham deleted the download_file_in_tutorial branch February 17, 2022 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation no-changelog-entry-needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants