-
-
Notifications
You must be signed in to change notification settings - Fork 237
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
Click errors for SQLFluff #1224
Comments
@nvuillam any ideas why the install doesn't use the latest click? We used to require |
@tunetheweb there is a pip install for all python packages I'm no pip expert, but probably another python package has a constraint on a lower version of click, compliant with sqlfluff dependency requirement defined in setup.cfg |
Yeah that's what worries me. So if our latest release demands a higher version will it not work? Or do you have a process to handle this in a separate env? |
Can't promise, but if it works with all the other packages, I'm pretty confident that it will work with sqlfluff :) |
SQLFluff 0.9.4 is released there if you wanna try it out? And if you could include in your next release that would be much appreciated! |
Auto-update job runs every night but for the occasion I just launched it, result in one big hour :) ( it will detect a new version and create a PR that will trigger test classes ) https://github.com/megalinter/megalinter/runs/4998717020?check_suite_focus=true |
Fingers cross we can get a release out and stop breaking you for once! Apologies for the errors in 0.9.2 and 0.9.3! |
When did you release the version that provokes the crash ? 5 hours ago, SqlFluff test cases worked perfectly fine -> https://github.com/megalinter/megalinter/runs/4997390251?check_suite_focus=true But maybe the sample file do not go thru the crashing code :/ |
@tdstark raised it to us after your last release, and it was an error I recognised whereby we discovered our issue. Not sure why your test suite didn't get it to be honest. That presumably runs in the combined Docker env so should be the same as what was released? |
@tunetheweb would you have some more extended working sql test files so I can replace the dramatically simple one that is used in MegaLinter tests ? :D |
Yeah let me try and reproduce with your sample SQLs. Would be good to understand this. |
I can reproduce it with that, and an old version of click:
Where Same with the following:
Are your tests run in isolation (in which case SQLFluff might have installed the latest as we only had a minimum and not a maximum version requirement) or are they run on the candidate combined Docker file (which may have used a different version if someone other package has a max dependency on |
Looks like it was snakefmt that requires the lower version btw:
That was downgraded yesterday: #1218. Could it be SQLFluff tests passed before this, then that downgrade happened, and hence the error? Or should all the tests be carried out on the final build? |
snakemake snakefmt generated a big mess between my latest tests of the beta and v5.7.0 releases.. I didn't have the choice but to downgrade them :/ It will probably be solved in a latest snakefmt release |
So that’s likely the reason your SQLFluff tests didn’t fail? Because of manual interventation? And normally it would have caught this? If so then at least we have an explanation. And I can hardly complain about last minute downgrades since I recently made you do one for SQLFluff! 😁 |
In CI , sqlfluff didn't fail with v0.9.3 and click v7.1.2 :) Isn't it the case it's supposed to fail ? 🤣 |
Yes. Thats why I’m confused. I just repeated running sqlfluff with click v7.1.2 and your test file and I get the fail. So why didn’t you? |
You're using python 3.8 in mac , MegaLinter is based on python:3.9.7-alpine3.13 on alpine linux ? :/ But that shouldn't make a difference :/ About your test of MegaLinter, you also add |
Yeah that shouldn’t matter, especially as error was seen with latest Mega Linter release.
That doesn’t matter. The delete statement is valid SQL in both ANSI (the default) and MySQL. My test above was completed with default ANSI without that param. |
When I look at the logs of the action: https://pipelines.actions.githubusercontent.com/CZNV7JsLKXAlORNSVk143wMHBd79LmRg3nz53BwEDVVX6AOjqA/_apis/pipelines/1/runs/52733/signedlogcontent/2?urlExpires=2022-01-30T22%3A02%3A47.1399361Z&urlSigningMethod=HMACV1&urlSignature=PpxcCawXOA%2BcYpKtOyPybtmF0wXRlBXazMVW9fBzFR0%3D it does look like it downloads different versions of click:
And then the SQLFluff tests were completed after that (so presumably on that final install of 8.0.3?):
That could be why it didn’t fail? Then the Image was built with 7.1.2 and that would make it fail. |
FWIW I'm still getting |
@tdstark do you have an example SQL file that triggers the error ? |
But no update has been released to include sqlfluff 0.9.4, so not surprising you're still getting the error. The only mystery is why the tests didn't catch it but, as per my comment above, it looks to me like the tests may not be set up to catch this. Perhaps they should be changed to also run on the final Docker image as well? |
@nvuillam it doesn't appear to even hit the sql. I get the following error and then the sql is skipped:
Here is my SQLFluff config in Github Actions:
|
Oh my mistake. I had it in my head for a moment that an update was pushed. |
@tunetheweb the tests are performed on the final docker image that has just been tested There has been numerous auto-updates for beta version, and none of them installed a sqlfluff version higher than 0.9.3 I have the same issue with black python formatter, that does not retrieves the latest version |
Then I'm so confused as to how that passed. Cause SQLFluff 0.9.3 is busted on old click, and looks like the Docker image uses old click :-(
I'm guessing it can't install it, cause of a dependency clash (e.g. we require a newer |
From a build of today:
....
.... and later in the same command log
... and finally, 0.9.3 is installed !
It seems something manages to get the lower version after having found the latest one... |
Arghhhhh I think I have just been welcome in dependency hell >< >< https://pip.pypa.io/en/stable/topics/dependency-resolution/#backtracking |
Yup. You are basically running this:
That is you don't specify a sqlfluff version - so it will get the latest it can. However do lock other packages, including This can be shown with the following by locking sqlfluff to 0.9.4 (and only installing snakefmt and sqlfluff):
And you see this error:
So, unless you have separate venvs you're gonna have to pick for now :-( Which is annoying as guess we both have breaking bugs... |
The following will work to exclude the bad sqlfluff versions btw:
But it does mean you're all the way back to 0.9.0 for now (until you remove that |
There's a lot to be said for node_modules that can handle this better rather than only having one shared space for dependencies! |
grmblmblmblmblm black and sqlfluff are much more used than snakemake... but I can not add a breaking change in a minor version, and v6 won't be ready yet for weeks..... node_modules handle it better.... but generates 500 megas of dependencies for each package :D I'll ask snakefmt if they can remove the |
For now just add this:
It'll install 0.9.0 (which works). Then it'll automatically upgrade to 0.9.4 when the dependency hell is broken.
True dat! And now you know why as each module takes it's own copy :-)
Looks like they insist on 7.1.2: https://github.com/snakemake/snakefmt/blob/master/pyproject.toml#L18 |
But after all this, I'm still confused as to why the tests passed! 0.9.3 is fundamentally broken if installed with an old click, so either:
|
I mean, FWIW I'd love the newest version of SQLFluff for all the improved dialect changes but that's just me. |
@tunetheweb I asked them, let's wait & see... :) snakemake/snakefmt#132 @tdstark you'll have it, and black is also widely used, and its latest version has been requested too :=) |
https://github.com/megalinter/megalinter/runs/5029301733?check_suite_focus=true I have to sleep, but except if bad luck, new beta version with latest sqlfluff and black will be available tomorrow :) |
And as a good news never comes alone, snakefmt maintainer is currently upgrading dependency :) |
Thanks for merging that fix. FYI we might be removing this strict requirement to allow us to play nicer with other (older) software: sqlfluff/sqlfluff#2547 |
Thanks for merging this! I tested the production branch is still broken but the beta branch appears to be operating as expected. |
I cleverly forgot to validate to release CI after creating 5.7.1... it will be ready in half an hour :) |
FYI SQLFluff 0.10.0 has just been released with less strict |
Great, thanks for the feedback :) |
Describe the bug
A clear and concise description of what the bug is.
Using SQLFLuff is producing errors like this:
To Reproduce
Steps to reproduce the behavior:
Add a workflow like this:
Expected behavior
A clear and concise description of what you expected to happen.
Screenshots
If applicable, add screenshots to help explain your problem.
Additional context
This is due to a bug upstream in SQLFluff in 0.9.3. We added a function only available in click 8.0 and above, but only updated our requirements.txt (used for local installs) and not our setup.cfg (used for
pip install
).We're releasing 0.9.4 now to fix it.
The text was updated successfully, but these errors were encountered: