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

Taking the latest of all. #120

Merged
merged 10 commits into from
Aug 13, 2024
Merged

Taking the latest of all. #120

merged 10 commits into from
Aug 13, 2024

Conversation

haz
Copy link
Contributor

@haz haz commented Aug 9, 2024

Proposed changes

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

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2024

⚠️ 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 ✅

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

❗ 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%) ⬇️

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%) ⬇️
pddl/helpers/base.py 97.80% <100.00%> (-0.03%) ⬇️
pddl/parser/problem.py 67.42% <ø> (-0.25%) ⬇️

... and 22 files with indirect coverage changes

@haz
Copy link
Contributor Author

haz commented Aug 9, 2024

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.

Copy link
Collaborator

@francescofuggitti francescofuggitti left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@haz
Copy link
Contributor Author

haz commented Aug 12, 2024

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.

@marcofavorito
Copy link
Member

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.

@haz
Copy link
Contributor Author

haz commented Aug 13, 2024

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...)

@marcofavorito
Copy link
Member

marcofavorito commented Aug 13, 2024

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}.

@haz
Copy link
Contributor Author

haz commented Aug 13, 2024

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

@francescofuggitti
Copy link
Collaborator

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.

@haz
Copy link
Contributor Author

haz commented Aug 13, 2024

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:

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 haz merged commit fef6c74 into main Aug 13, 2024
9 checks passed
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