-
Notifications
You must be signed in to change notification settings - Fork 35
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
CI: Pandas remove #717
CI: Pandas remove #717
Conversation
@AdamTheisen @mgrover1 Thoughts on adding a pytest-timeout module to pass on the get_ord tests if it goes longer then 5 minutes? This happens every PR for atleast one of the suites. Granted I don't know why the io_write stalls for some tests sometimes |
cca44bb
to
7e5ec80
Compare
I think this brings up a bigger issue here - we should add sample files to test on and not rely on hitting these servers since a suite of python versions + machines will often overload their bandwidth. I would rather have a single file we test against that we know is on a system with improved bandwidth than pass when things timeout |
@mgrover1 @zssherman I thought we added a timeout to that test already but maybe not. I think we should discuss the file part more. We've had cases in the past where the API updated and it was only through these tests that we would find it. If we are going off a static file we could be using outdated formats. Thoughts? |
@AdamTheisen @mgrover1 Yeah I agree on testing the API so we know if something changes. However I also agree with bandwidth issues as well, is there a way to ping the API and not download the file to just see if the path is valid? Also we did not have the timeout, also I believed it was suppose to skip it if it took to long from the documentation, but it looks like it failed the testing suite which isn't what I wanted... so the timeout might not work anyways, removing timeout for now. |
32d95b6
to
c2963ab
Compare
@AdamTheisen I removed the pytest timeout stuff, are we good to merge this? This will fix the CI for some of the other PRs I have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zssherman please go ahead!
Had to reopen PR as PR wasn't updating to new commits (First for everything). Also add timeout for get_ord stalling