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

new build for deafrica s2 collection upgrade #145

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

abradley60
Copy link
Contributor

  • New build to incorporate changes from new release of odc-apps-dc-tools (0.2.17 -> 0.2.18)
  • This is required to index new sentinel-2 collection (s2_l2a_c1) with argo workflows

index/readme.md Outdated
To minimise version changes, update using the existing image:

```
docker run -v $(pwd):/datacube-index/ -w /datacube-index -it opendatacube/datacube-index bash -c "pip install pip-tools && pip-compile --upgrade --output-file constraints.txt requirements.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you replace pip install with python3 -m pip install, we're guaranteed to use the system Python.

It probably doesn't make a difference in this case, but people often copy and paste whatever they have in front of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjonsson thanks will update

index/constraints.txt Outdated Show resolved Hide resolved
@@ -205,7 +199,7 @@ numpy==1.26.4
# xarray
odc-apps-cloud==0.2.3
# via -r requirements.txt
odc-apps-dc-tools==0.2.17
odc-apps-dc-tools==0.2.18
# via -r requirements.txt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requirements.txt hasn't been touched. The odc-apps-dc-tools>=0.2.18 pin should go in there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Paul, it's pulling the latest version by default. Should I fix the version in the requirements.txt itself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you re-ran pip-compile off requirements.txt it in an environment that already had an older version of odc-apps-dc-tools installed, it would stick with the existing version.

If you actually have minimum version requirements they should go in requirements.txt. It won't break anything as is, but is good practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I am running the install with the --upgrade flag which will upgrade all of the packages in the requirements.txt. This was documented in the Readme so it's how I applied it.

Should I :

  1. run again without the upgrade flag and fix the version in the requirements.txt?
  2. keep it as is so other packages are upgraded as well?

Copy link
Contributor

@jmettes jmettes Jul 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was curious to learn more about pip's behaviour in these conditions, so ran some experiments to see what would happen.

TLDR:

  • Always run with --upgrade
  • Delete constraints.txt, when not using --upgrade
  • Pinning requirements.txt with >= seemed to have little benefit

$ python -m venv env
$ source env/bin/activate
$ python --version
3.11.9

$ pip install pytest==8.3.1 # old version, latest is 8.3.2
$ pip install pip-tools
$ python
>>> import pytest
>>> pytest.__version__
'8.3.1'

$ echo "pytest" > requirements.txt  # note, no version pinned
$ pip-compile --output-file constraints.txt --strip-extras requirements.txt
#
# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
#    pip-compile --output-file=constraints.txt --strip-extras requirements.txt
#
iniconfig==2.0.0
    # via pytest
packaging==24.1
    # via pytest
pluggy==1.5.0
    # via pytest
pytest==8.3.2
    # via -r requirements.txt

it still retrieved latest version, inside an environment with old version, even without --upgrade or pinning >= in requirements.txt

let's try simulating having an old constraints.txt file lying around:

$ sed -i 's/pytest==8.3.2/pytest==8.3.1/' constraints.txt 
$ pip-compile --output-file=constraints.txt --strip-extras requirements.txt

$ pip-compile --output-file=constraints.txt --strip-extras requirements.txt
#
# This file is autogenerated by pip-compile with Python 3.11
# by the following command:
#
#    pip-compile --output-file=constraints.txt --strip-extras requirements.txt
#
iniconfig==2.0.0
    # via pytest
packaging==24.1
    # via pytest
pluggy==1.5.0
    # via pytest
pytest==8.3.1
    # via -r requirements.txt

here it does keep old version

$ rm constraints.txt 
$ pip-compile --output-file=constraints.txt --strip-extras requirements.txt
...
pytest==8.3.2

removing old constraints.txt lets it pick it up again!

$ pip-compile --upgrade --output-file constraints.txt --strip-extras requirements.txt
$ sed -i 's/pytest==8.3.2/pytest==8.3.1/' constraints.txt 

$ pip-compile --upgrade --output-file constraints.txt --strip-extras requirements.txt
...
pytest==8.3.2

--upgrade still lets it pick it up new version, even if old constraints.txt is hanging around

$ echo 'pytest>=8.3.1' > requirements.txt
$ sed -i 's/pytest==8.3.2/pytest==8.3.1/' constraints.txt 
$ pip-compile --output-file constraints.txt --strip-extras requirements.txt
...
pytest==8.3.1

pinning requirements.txt with >= has no effect, when old constraints.txt present

@jmettes jmettes merged commit 160d5e9 into main Jul 26, 2024
3 checks passed
@jmettes jmettes deleted the deafrica-s2-collection-index branch July 26, 2024 04:56
@pjonsson
Copy link
Collaborator

@alexgleith @SpacemanPaul this bumps version.txt so I've drafted a 0.3.4 release, please check/edit and publish.

@pjonsson pjonsson mentioned this pull request Aug 7, 2024
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.

4 participants