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

chore(python): ensure that make requirements fully refreshes unpinned packages/deps #10591

Merged

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Aug 18, 2023

Until running the equivalent commands I was getting some mysterious new typing errors from pre-commit checks that a vanilla make requirements did not fix. Have confirmed locally that this properly refreshes unpinned packages/deps, ensuring that your local env will match what runs in CI (which does a fresh install).

If preferred, could make this a separate option, say make requirements-clean? (Though given that, post initial install, you're likely to want it to be thorough I'm not sure there's any real value in two options - I'd say just make this one robust by default unless there's a compelling reason otherwise 🤔).

@github-actions github-actions bot added enhancement New feature or an improvement of an existing feature python Related to Python Polars rust Related to Rust Polars labels Aug 18, 2023
@alexander-beedie alexander-beedie added the build Changes that affect the build system or external dependencies label Aug 18, 2023
@alexander-beedie alexander-beedie changed the title feat: ensure that make requirements fully refreshes unpinned packages/deps fix(build): ensure that make requirements fully refreshes unpinned packages/deps Aug 18, 2023
@alexander-beedie alexander-beedie removed the rust Related to Rust Polars label Aug 18, 2023
@github-actions github-actions bot added rust Related to Rust Polars fix Bug fix labels Aug 18, 2023
@stinodego
Copy link
Member

I like the idea of make requirements upgrading packages to the latest unpinned versions. But isn't just adding --upgrade enough to achieve that? In other words, why would we need --force-reinstall and --no-deps here?

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Aug 18, 2023

I like the idea of make requirements upgrading packages to the latest unpinned versions. But isn't just adding --upgrade enough to achieve that? In other words, why would we need --force-reinstall and --no-deps here?

Observationally --upgrade by itself didn't seem to do the trick; as for --no-deps that was something I was trying locally and didn't mean to include - removing! 😅 (It seemed like we could have a slightly smaller set of required packages in CI by omitting the deps; it works, but could bite us later for only marginal gain).

@stinodego
Copy link
Member

Observationally --upgrade by itself didn't seem to do the trick

This really puzzles me, how could that be?

I'd rather not have --force-reinstall in there - seems to me it's just wasting compute and the developers time. If the dependencies are up to date, leave them be.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Aug 18, 2023

Well, we can try it without; maybe you'll have more luck than me. Perhaps it does a better job resolving from scratch after having uninstalled everything, or it had somehow got in a bad state? 🤷 I'll take it out for now and see how it goes - and/or we could add the extra flag as make requirements-clean in case the nuclear option is required?

@stinodego stinodego changed the title fix(build): ensure that make requirements fully refreshes unpinned packages/deps chore(python): ensure that make requirements fully refreshes unpinned packages/deps Aug 18, 2023
@stinodego stinodego removed the build Changes that affect the build system or external dependencies label Aug 18, 2023
@github-actions github-actions bot added the internal An internal refactor or improvement label Aug 18, 2023
@stinodego stinodego added internal An internal refactor or improvement and removed enhancement New feature or an improvement of an existing feature internal An internal refactor or improvement rust Related to Rust Polars fix Bug fix labels Aug 18, 2023
@stinodego
Copy link
Member

stinodego commented Aug 19, 2023

Well, we can try it without; maybe you'll have more luck than me. Perhaps it does a better job resolving from scratch after having uninstalled everything, or it had somehow got in a bad state? I'll take it out for now and see how it goes

Indeed let's do that. If this for some reason does not do the trick, get a pip list of the problematic environment and we can see what's really going on. We can always add --force-reinstall later if it turns out to be needed in some edge case.

we could add the extra flag as make requirements-clean in case the nuclear option is required?

We already have a nuclear option in make clean && make build, so I don't think that will be necessary.

@alexander-beedie alexander-beedie merged commit d7bc251 into pola-rs:main Aug 19, 2023
11 checks passed
@alexander-beedie alexander-beedie deleted the enhance-make-requirements branch August 19, 2023 06:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants