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

Use pixi (attempt 2) #222

Merged
merged 63 commits into from
Sep 21, 2024
Merged

Use pixi (attempt 2) #222

merged 63 commits into from
Sep 21, 2024

Conversation

cisaacstern
Copy link
Collaborator

A fresh take on the work in #220

@cisaacstern
Copy link
Collaborator Author

I am very happy where this is, and definitely see this as the basis for the path forward.

Next steps (as PR's on top of / following on this):

@cisaacstern
Copy link
Collaborator Author

More TODO:

  • In merging Map legends #223 I realized that the existing vendor version check doesn't sufficiently capture possible changes to the dependencies of ecoscope and/or lithops ... lonboard fork is bumped to 0.0.3 and we are not catching that in CI. Add some mechanism to prevent this going unnoticed.

colors=legend_colors,
**legend_style.model_dump(exclude_none=True), # type: ignore[union-attr]
colors=[str(lc) for lc in legend_colors],
**(legend_style.model_dump(exclude_none=True)), # type: ignore[union-attr]
Copy link
Collaborator Author

@cisaacstern cisaacstern Sep 11, 2024

Choose a reason for hiding this comment

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

🤔 @atmorling I was really struggling to get the tests to pass following merge of #223 and ultimately found I needed this change to avoid the error

FAILED tests/tasks/test_ecomap.py::test_draw_ecomap_with_legend - traitlets.traitlets.TraitError: The 'colors' trait of a LegendWidget instance contains an Unicode of a List which expected a unicode string, not the tuple (0, 255, 0, 255).

Does this change seem ok? i.e.

colors=[str(lc) for lc in legend_colors],

instead of

colors=legend_colors,

And if so, any idea why tests were passing on merge of #223 but not here? I really don't think I've changed anything that would touch this 🫤 ... unless the vendored conda edition of lonboard has a some dependency version difference that matters?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

unless the vendored conda edition of lonboard has a some dependency version difference that matters?

All CI dependencies for this PR are listed in the lockfile, e.g. for one of the test envs on which I was getting the Traitlets error, packages are listed here:
https://github.com/wildlife-dynamics/ecoscope-workflows/blob/fe1d1ba64722ddd4f0f56306663de8e40b80d968/pixi.lock#L7392-L7403

Choose a reason for hiding this comment

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

Just looking at pyh4616a5c_5 build, it looks like that was done against v1.8.3 of ecoscope and not the latest in master (which is what #223 points at). It looks like pyh4616a5c_4 was actually built from the "correct" git ref (basing this off the version of the lonboard fork)

In any case this all probably stems from the fact that #223 links to a commit rather than a tagged release, this was to streamline for demos and not something I intend to make a habit of.
I've tagged v1.8.4 which should reduce the headache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for these insights! I'm now tracking this in #244

@cisaacstern cisaacstern changed the base branch from main to cs/vipingo September 21, 2024 21:43
@cisaacstern cisaacstern marked this pull request as ready for review September 21, 2024 21:46
@cisaacstern cisaacstern merged commit 41d0db3 into cs/vipingo Sep 21, 2024
28 checks passed
@cisaacstern cisaacstern deleted the use-pixi branch September 21, 2024 21:56
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.

2 participants