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

Pin pytables to match version installed through conda #1139

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

merelcht
Copy link
Member

@merelcht merelcht commented Jan 6, 2022

Description

The win_unit_tests_3.8 are failing on several PRs with a Window stackoverflow error which seems to be related to pytables. Pytables have released a new version 9 days ago, but it’s not on conda-forge, so in our windows CI we install pytables 3.6.1 through conda install -c conda-forge pytables -y and then in a later step when all requirements are installed pip installs the latest pytables version, which is now 3.7.0.

Development notes

I’m not entirely sure why the errors are only happening on 3.8 (this might be related: PyTables/PyTables#933), but pinning tables in test_requirements.txt seems to fix it for now.

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes

@merelcht merelcht requested a review from idanov as a code owner January 6, 2022 11:50
@merelcht merelcht self-assigned this Jan 6, 2022
Copy link
Contributor

@lorenabalan lorenabalan left a comment

Choose a reason for hiding this comment

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

We can pin it just for windows but it's only a test requirement so I'm not that bothered.

@antonymilne
Copy link
Contributor

antonymilne commented Jan 6, 2022

Thanks for doing this! Looks like a good fix for the windows tests, but should we be changing this requirement too (just for windows like @lorenabalan suggested)? https://github.com/quantumblacklabs/kedro/blob/main/setup.py#L80

I'm not sure whether we should or not. On the one hand, would be nice to keep things consistent, but on the other since this wouldn't be released for a while maybe we should just wait until pytables have come out with a fix and then change to that new version. I don't know.

@merelcht
Copy link
Member Author

merelcht commented Jan 6, 2022

Yep good points, I'll specify the operating system on the requirements and update setup.py as well.

@merelcht merelcht merged commit 61319e2 into main Jan 7, 2022
@merelcht merelcht deleted the fix/windows-38-pytables branch January 7, 2022 13:32
rashidakanchwala pushed a commit that referenced this pull request Jan 10, 2022
Signed-off-by: Rashida Kanchwala <rashida.kanchwala@quantumblack.com>
doytsujin pushed a commit to doytsujin/kedro that referenced this pull request Jan 22, 2022
…kedro-org#1139)

* Update deployment docs to use direct call to pipelines instead of through context

* Fix lint
lvijnck pushed a commit to lvijnck/kedro that referenced this pull request Apr 7, 2022
Signed-off-by: Laurens Vijnck <laurens_vijnck@mckinsey.com>
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.

5 participants