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

Compatibility with networkx>=2.0 and python 3.7+ #68

Merged
merged 8 commits into from Apr 26, 2022
Merged

Compatibility with networkx>=2.0 and python 3.7+ #68

merged 8 commits into from Apr 26, 2022

Conversation

pylanglois
Copy link
Contributor

Copy link
Owner

@brooksandrew brooksandrew left a comment

Choose a reason for hiding this comment

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

@pylanglois - thanks for the PR!

Given latest networkx releases cut support for earlier versions of python, I'm generally onboard with this lib following suit.

It's been a while since new contributions to this lib and my Travis CI has been deactivated on the .org domain. Running into some issues migrating to the new .com domain... but once i get that up and tests running again through CI. will merge this.

Thanks again for helping keep this lib current w the times 👍

https://networkx.org/documentation/stable/release/release_2.6.html

setup.py Outdated
install_requires=[
'pandas',
'networkx==2.0'
'networkx'
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like networkx 2.6 supports python 3.7, 3.8, 3.9. Any reason to restrict to >=3.8 ?

https://networkx.org/documentation/stable/release/release_2.6.html

Copy link
Owner

Choose a reason for hiding this comment

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

Also if we pin to specific python versions, perhaps also pin networkx with >= to the earliest version supported by your changes? Any reason not to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I usually prefer to support the latest version of all package and the documentation of networkx only contains version 2.7.1 And, I'm not used to pin version but if you want to pin to 2.6 it's ok, I don't really have a good reason not to do it.

Copy link
Owner

Choose a reason for hiding this comment

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

My thought was just pin to greater than or equal to the known minimum working version. Ex. networkx >= 2.4 if we know this package will not work with 2.3.

So we still get the latest version by default

postman_problems/solver.py Show resolved Hide resolved
@brooksandrew
Copy link
Owner

@brooksandrew
Copy link
Owner

Re-opening PR to trigger Travis CI

@brooksandrew brooksandrew reopened this Mar 22, 2022
@brooksandrew
Copy link
Owner

@pylanglois - OK i got in touch with the Travis CI folks and got this project designated as an OSS which qualifies for enough compute credits to turn Travis CI back on to maintain this project. hooray. Just re-triggered a build on your PR which should now build on each new commit to it.

Can you access Travis CI to see the logs from each CI run? Not sure of the permissions for folks that are not me. If you can get those tests passing (only 2 of 34 failed), I'd like to merge this.

https://github.com/brooksandrew/postman_problems/pull/68/checks?check_run_id=5646750303

image

@pylanglois pylanglois changed the title Compatibility with networkx>=2.6 and python 3.8+ Compatibility with networkx>=2.0 and python 3.7+ Mar 23, 2022
@pylanglois
Copy link
Contributor Author

I tested with python 3.7.12 and it work locally... I will learn how to run Travis CI locally and then commit version that pass the tests.

@brooksandrew
Copy link
Owner

@pylanglois - are you able to see the Travis CI runs (screenshot below) w details on the tests passing/failing? Your latest commit looks good as it is passing Python 3.8 and 3.9. It looks like some weird dependency issue with 3.7 and coveralls. And 3.10 looks like it might not be available on Travis. So I'd be OK to just comment out 3.7 and 3.10 in the travis configuration for now. And then we can merge your upgrade in.

image

@brooksandrew
Copy link
Owner

hi @pylanglois - just following up. Per the last comment, I'd be OK to test this on 3.8 and 3.9 only for now in Travis CI. If that passes, lets merge this.

@pylanglois
Copy link
Contributor Author

Ok. I started a little experiment with github actions. I will close that shortly and go with your suggestion.

@pylanglois
Copy link
Contributor Author

hi @brooksandrew,

The tests I ran seems to be ok for all python versions above 3.7.1. See: https://github.com/pylanglois/postman_problems/actions/runs/2204057017

Do I put python_requires='>=3.7.1' as requirement?

@pylanglois
Copy link
Contributor Author

pylanglois commented Apr 25, 2022

If you are interested, I can also include the github action pipeline:
https://github.com/pylanglois/postman_problems/blob/pylanglois/github_actions/.github/workflows/python-app.yml

Copy link
Owner

@brooksandrew brooksandrew 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 for the ping on this. Saw it when on mobile when i was away from computer and forgot to return to it.

And double thanks for verifying that the test suite runs on 3.7 and 3.10. I'm not sure what's going on with Travis CI machines, but that's good to know that most users should be fine with >3.7, so I'm good with the python version as you have it.

@brooksandrew brooksandrew merged commit 4b2ba0c into brooksandrew:master Apr 26, 2022
@pylanglois pylanglois deleted the pylanglois/py38 branch April 27, 2022 02:12
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.

None yet

2 participants