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

Update docs (Closes #110) #111

Merged
merged 30 commits into from
Nov 16, 2022
Merged

Update docs (Closes #110) #111

merged 30 commits into from
Nov 16, 2022

Conversation

hmgaudecker
Copy link
Member

No description provided.

@hmgaudecker hmgaudecker marked this pull request as draft October 18, 2022 06:15
@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Merging #111 (69e19a4) into master (f773782) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 69e19a4 differs from pull request most recent head aff2e67. Consider uploading reports for the commit aff2e67 to get more accurate results

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           55        55           
=========================================
  Hits            55        55           
Flag Coverage Δ
end_to_end 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/test_cookie.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@timmens
Copy link
Member

timmens commented Oct 18, 2022

I updated the .pre-commit-config.yaml to use modern formatters on the documentation. Unfortunately, this also changed a few non-documentation files.

@timmens
Copy link
Member

timmens commented Oct 27, 2022

I updated the build process for the documentation. Now you can run make html and it executes the pytask script that generates the documentation relevant figures before running sphinx-build. This is just an added line to the Makefile and can be easily reverted.

@hmgaudecker
Copy link
Member Author

I updated the build process for the documentation. Now you can run make html and it executes the pytask script that generates the documentation relevant figures before running sphinx-build. This is just an added line to the Makefile and can be easily reverted.

Cool, thanks! I am not sure I fully understand why the figures are still present in the repo then? If they are re-created each time make html runs (as they should in an ideal world), we could just delete them?

@timmens
Copy link
Member

timmens commented Oct 28, 2022

Yes, I missed that. They are deleted now!

@hmgaudecker
Copy link
Member Author

Hi @timmens, thanks again for all your work! I went through everything now, looks great! Just a couple of wishes / updates:

  • In 51c12b I changed the LaTeX pipeline to what you would do in a serious project. This likely broke CI again (ignore the commit before that, had a stupid mistake in that one). We need to infer LaTeX deps automatically etc., can't tell users that they should change that on their own. I think the way forward would be to add some conditionality depending on whether we are in a CI environment (maybe we need to add a cookiecutter option that we tell users to ignore), that should also help with the inability to run the full test pipeline on local machines?
  • Also in that commit, I removed the paths set for LaTeX (too much action at a distance... Better be explicit with paths in documents). However, the tables don't show up in my DAG anymore, not sure why, could you investigate, please?
  • I think we should add the data to the project rather than downloading it. Else people will think pytask would check for changes in the remote data, too, which is not what is happening. Please also add the precise source to the "abstract" in running_example.

Thanks!!!!

timmens and others added 7 commits November 15, 2022 11:43
* Move to Python 3.11

* Add conda-build to environment again

* Update environments

* Update conda-incubator arguments to run with Py3.11

* Update some version numbers.

* Use 3.11 as default in Github Actions matrix, remove 3.8

* Switch to python-kaleido and use 3.10 for docs

* Remove Python 3.7 and 3.8 from setup.cfg

* Use pip Kaleido for documentation

* Use Python 3.9 for documentation

Co-authored-by: Hans-Martin von Gaudecker <hmgaudecker@gmail.com>
@timmens
Copy link
Member

timmens commented Nov 15, 2022

@hmgaudecker

I handled the first and third tasks. I didn't manage to find a solution for the second. It seems that pytask has to do stuff with the task before inferring the dependencies. I will talk to Tobi about this next time I see him.

We have one more problem. Automatic generation of the images that are used in the documentation is not possible after all. I misused the conf.py file to run pytask before readthedocs starts building the documentation. However, some images are generated using latex, which is not installed on the servers used by readthedocs. I think the best solution is that we simply upload the generated pictures. What do you think?

@hmgaudecker
Copy link
Member Author

Yes, cool, perfect solution.

Only annoying thing for me is running the tests locally from a dedicated environment, I get:

FAILED tests/test_cookie.py::test_check_conda_environment_creation_and_run_all_checks - subprocess.CalledProcessError: Command '('bash', '-l', '-c', 'conda run -n __test__ pre-commit run --all-files')' returned non-zero exit status 1.

EnvironmentLocationNotFound: Not a conda environment: /home/xxx/miniconda3/envs/econ-project-templates/envs/__test__

The environment is there, but in a different location:

$ conda env list
# conda environments:
#
base                     /home/xxx/miniconda3
__test__                 /home/xxx/miniconda3/envs/__test__

Do you see a quick solution for that? Else I'd be fine with merging this as is, thanks for all your work!

@hmgaudecker hmgaudecker marked this pull request as ready for review November 15, 2022 16:53
@timmens
Copy link
Member

timmens commented Nov 15, 2022

On my machine (macOS) it runs locally (just tried). I haven't checked on my Linux. If it is a Windows problem then I don't have a quick solution.

I can check if it runs on my Linux tomorrow. If yes I'd say we merge and open an issue.

@hmgaudecker
Copy link
Member Author

Ubuntu 22.04 -- key is to start pytest from the econ-project-templates environment.

$ conda --version
conda 22.9.0

@timmens timmens merged commit b488cd5 into master Nov 16, 2022
@timmens timmens deleted the update-docs branch November 16, 2022 09:37
@hmgaudecker hmgaudecker mentioned this pull request Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants