pddl icon indicating copy to clipboard operation
pddl copied to clipboard

Taking the latest of all.

Open haz opened this issue 1 year ago • 3 comments

Proposed changes

Testing out what would happen if we just take the latest of every package in the dev requirements.

haz avatar Aug 09 '24 17:08 haz

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.98%. Comparing base (8e3be08) to head (d7400f0).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
- Coverage   88.37%   87.98%   -0.39%     
==========================================
  Files          25       24       -1     
  Lines        1798     1773      -25     
  Branches      333      333              
==========================================
- Hits         1589     1560      -29     
- Misses        149      152       +3     
- Partials       60       61       +1     
Flag Coverage Δ
unittests 87.98% <100.00%> (-0.39%) :arrow_down:

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

Files Coverage Δ
pddl/core.py 91.56% <100.00%> (-1.85%) :arrow_down:
pddl/helpers/base.py 97.80% <100.00%> (-0.03%) :arrow_down:
pddl/parser/problem.py 67.42% <ø> (-0.25%) :arrow_down:

... and 22 files with indirect coverage changes

codecov-commenter avatar Aug 09 '24 18:08 codecov-commenter

Thoughts on nuking the specific versions? We have a long list of auto-generated PR's, and this PR seems to bring us back up to snuff.

I had to ignore a single vulnerability listed in Jinja2: https://data.safetycli.com/v/70612/97c/

It's a bit odd, as it's listed for every version of the package (>=0). Also created half a decade ago and updated only 3 days ago.

Created at Feb 15, 2019 Updated at Aug 06, 2024

So I'm assuming it's just a bug with the bug database?

If we don't want * versions on the packages (it's all the dev packages -- not lark+click), then we'll need to find a way to update them to reasonable ranges.

haz avatar Aug 09 '24 20:08 haz

Not even any discussion? ;)

Feels like a pretty drastic move to just accept the latest version, but as long as we keep checking in, it should be alright.

haz avatar Aug 12 '24 15:08 haz

Not even any discussion? ;)

Feels like a pretty drastic move to just accept the latest version, but as long as we keep checking in, it should be alright.

LGTM too.

In the end, Pipfile does not affect the package's dependencies, as the only dependencies are lark and click in setup.py, and their version requirements are opportunely upper-bounded.

However, there might be issues with the software's behavior in the local development environment and the environments of GitHub Actions CI. It often happened to me in other projects that CI jobs broke in weird and subtle ways due to a recent release of a random (sub-)dependency in the dependency tree of the local dev env, which nevertheless was compatible with the version requirements in the Pipfile.

On the upside, this incentivizes a more frequent refresh of the local dev environment in case of inconsistencies between local environments among developers/contributors and CI environments.

Questions:

  • I see that in tox.ini, the version requirements for the development dependencies are unchanged. This means that, when using the Python interpreter managed by pipenv, the local execution of dev jobs (lint/static checks, tests etc.) might give different results one on CI or when using tox because the latter might point to older versions of the dev dependencies.
  • The (sub-)dependencies locked via pipenv might be incompatible with the (sub-)dependencies of the pddl package because lark and click are not included in the Pipfile. Perhaps we could just add lark and click in the Pipfile with the same version requirements as in setup.py.

marfvr avatar Aug 13 '24 15:08 marfvr

Both excellent questions/points! I've got a few min before my next meeting, so will take a crack. How would you advise I test the tox.ini changes locally? (don't think the CI/CD would run through it, but could be wrong...)

haz avatar Aug 13 '24 15:08 haz

Both excellent questions/points! I've got a few min before my next meeting, so will take a crack. How would you advise I test the tox.ini changes locally? (don't think the CI/CD would run through it, but could be wrong...)

The current GH workflow already runs the checks using tox: https://github.com/AI-Planning/pddl/blob/main/.github/workflows/test.yml#L27-L30 .

Locally, you can run tox -e py3X (where X is one of {7, 8, 9, 10}.

marfvr avatar Aug 13 '24 15:08 marfvr

Chasing down issues with lazy fixtures...almost there...

haz avatar Aug 13 '24 16:08 haz

Not even any discussion? ;)

Feels like a pretty drastic move to just accept the latest version, but as long as we keep checking in, it should be alright.

Sorry about that! I aimed to ease the process instead of blocking it since everything worked out.

Here's my take on the topic: when working with pipenv and the Pipfile, I’ve often encountered issues with matching the right package versions. Having PRs waiting for a long time without being merged or closed can be quite frustrating. They frequently cause dependency issues, which are often subtle (as Marco is pointing out) and hard to debug. This significantly slows down the development workflow and makes the repo feel abandoned (this may discourage external contributions).

With that in mind, I'd suggest simplifying package versioning using alternative dependency management systems or a simple project.toml. However, I understand that this change is quite significant, and we don’t want to jump the gun on it.

francescofuggitti avatar Aug 13 '24 16:08 francescofuggitti

Ok, mistune had a major revision change, and had to make modifications like this and this.

Also, pytest-lazy-fixture went defunct, and was picked up by another initiative:

  • https://www.mail-archive.com/[email protected]/msg105447.html
  • https://github.com/TvoroG/pytest-lazy-fixture/issues/65
  • https://github.com/dev-petrov/pytest-lazy-fixtures
  • https://github.com/AI-Planning/pddl/pull/120/files#diff-e52e4ddd58b7ef887ab03c04116e676f6280b824ab7469d5d3080e5cba4f2128R20

Can confirm that a fresh tox works locally. Should now be set to approve+merge!

Update: Just read @francescofuggitti's response. A more unified might work, ya. For now, I'm ok with dependency hunting to keep us up-to-date with the testing side. The important part is deployment, and we're locked into only two dependencies there. If we ever want to scale back and lock in versions, we can just look to the Pipfile.lock for what's currently working :P

haz avatar Aug 13 '24 17:08 haz