I167 upgrade python #168
I167 upgrade python #168
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Ended up having to make an adjustment to |
| @bash -c '${PYTHON} -m pytest -v tests/' | ||
| @bash -c 'poetry run pytest -v tests/' | ||
|
|
||
| .PHONY: test-not-online |
There was a problem hiding this comment.
This target should also be documented in the 'help' target
Makefile
Outdated
| @-python3 setup.py sdist | ||
| @-python3 setup.py bdist_wheel | ||
| @bash -c 'poetry build -f sdist' | ||
| @bash -c 'poetry build -f wheel' |
There was a problem hiding this comment.
poetry build defaults to both sdist and wheel. No need to specify them.
| @@ -34,7 +34,7 @@ | |||
| "name": "stdout", | |||
| "output_type": "stream", | |||
| "text": [ | |||
| "Using thunderbird on https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thunderbird/wps\n" | |||
| "Using thunderbird on https://marble-dev01.pcic.uvic.ca/twitcher/ows/proxy/thunderbird/wps\n" | |||
There was a problem hiding this comment.
The host is subject to change (you just did it). Is there some way for this to be set programmatically, or from an env var (THREDDS_HOST)?
There was a problem hiding this comment.
It's also possible this file is the output of some automated process, if so, ignore my comment.
| url = "https://pypi.pacificclimate.org/simple" | ||
| priority = "supplemental" | ||
|
|
||
| [tool.poetry.dependencies] |
There was a problem hiding this comment.
I see some exact version specs here (e.g., cftime = "1.6.3"), which are not desirable unless they are necessary to prevent installation errors. Please check whether you can loosen them.
It looks as if these are all or mostly top-level deps, yay. However, if there are any subsidiary deps, please see if you can remove them. Minimal dependencies are a good goal.
| cdo = "1.6.0" | ||
| poetry-dotenv-plugin = "^0.2.0" | ||
|
|
||
| [tool.poetry.group.dev.dependencies] |
| requires = ["poetry-core"] | ||
| build-backend = "poetry.core.masonry.api" | ||
|
|
||
| [tool.pytest.ini_options] |
thunderbird/utils.py
Outdated
| @@ -52,4 +53,4 @@ def get_url(): | |||
| if url: | |||
| return url | |||
|
|
|||
| return "https://docker-dev03.pcic.uvic.ca/twitcher/ows/proxy/thunderbird/wps" | |||
| return "https://marble-dev01.pcic.uvic.ca/twitcher/ows/proxy/thunderbird/wps" | |||
There was a problem hiding this comment.
Again, it would be best if this were obtained from an env var or other external source.
Makefile
Outdated
| DEV_PORT ?= $(shell bash -c 'read -ep "Target port: " port; echo $$port') | ||
|
|
||
| # Used in target refresh-notebooks to make it looks like the notebooks have | ||
| # been refreshed from the production server below instead of from the local dev | ||
| # instance so the notebooks can also be used as tutorial notebooks. | ||
| OUTPUT_URL = https://docker-dev03.pcic.uvic.ca/wpsoutputs | ||
| OUTPUT_URL = https://marble-dev01.pcic.uvic.ca/wpsoutputs |
.cruft.json
Outdated
There was a problem hiding this comment.
Suggest finding out what this suspiciously named file is for, and remove it if it truly is cruft.
.env
Outdated
| @@ -0,0 +1 @@ | |||
| THREDDS_HOST="marble-dev01.pcic.uvic.ca" | |||
There was a problem hiding this comment.
Get host name from this env var wherever you can in other code, to avoid hardcoding the value. I've added a few comments to that effect below, but there may be other places where it could be done.
|
And one final remark, as we discussed a couple of minutes ago: If Black is going to reformat a lot of the project, not code you are actually modifying the logic/function of, do a separate Black PR (before your coding PR). The Black PR won't need reviewing. |
This PR resolves #167. It makes the following changes to the package:
piptopoetryand modifies Dockerfile, Makefile, CI actions, and user/dev docs accordingly3.8to3.10docker-dev03.pyfiles with the newer version ofblackUsing the
makecommand, I have verified that the installation process, automated tests intests/, and notebooks all work correctly.