Feature/upgrade to networkx 2 #1509
Conversation
|
I'm investigating the failures now... |
|
Hey @heisencoder - thanks for making this PR!! I think you'll want to rebase this one against the latest version of We don't automatically run integration tests when PRs are submitted. These things run against our internal Snowflake/Redshift/BQ databases, so someone nefarious could log our credentials if they were so inclined :). You mentioned in the commit message that the unit tests are passing. Let me take a quick look at this one, then I can try to kick off tests for you |
|
After running some local unit tests against networkx 1.11, I discovered that the migration guide lied about it being sufficient to just drop the _iter suffix on in_degree_iter. That said, I've got a work-around that works in both networkx 1 and 2: def _find_new_additions(self):
for node, in_degree in dict(self.graph.in_degree()).items():I had based this change on the dev/Louisa May Alcott because #1496 was tagged with Louisa May Alcott, but I'm happy to rebase this against the main development branch. |
|
🤦♂ you're totally right! I'm actually not certain that we want to merge this into |
|
Looks like I've got the merge messed up right now, so I'll need to look into getting it fixed. So, to conclude, I'm assuming that rebasing against dev/wilt-chamberlain is the best way forward? This seems to have the least friction against the pull-request system. For me, it doesn't matter too much which release this ultimately gets into, since I'm maintaining a local patch to unblock my work. |
|
In any case, version 71e3f46 has the full change in it. |
Also, update test_linker.py to run under Python 2 unit tests and add test coverage for the modified functions. linker.py should now be compatible with both networkx-1 and -2. This addresses #1496 This change was tested via unit tests, but was not tested via integration tests. As a follow-up change, core/setup.py should have its networkx dependency updated to allow version 2.x.
|
I think I've successfully wrestled git to now make this pull request be a single commit off of dev/wilt-chamberlain! The only thing left would be to update core/setup.py to allow for networkx version 2.x. I'm happy to make that change as a second commit in this branch depending on how this will fit into the bigger picture of dbt releases. Please feel free to review this version. Thanks! |
|
I went ahead and bumped the networkx version in core/setup.py in change 7ed0036 |
|
Looks like I need to also make some changes to test_graph.py. I'll dig in further to see what else has been affected. Current error:
E AssertionError: NodeView(('model.test_models_compile.model_one',)) != ['model.test_models_compile.model_one'] (networkx 2 is using a tuple instead of a list in this case) |
|
Hey @heisencoder! It looks like you're seeing issues between networkx 1.11 vs 2.x - you should feel comfortable dropping support for 1.x entirely, I think that will make life easier for everyone! |
|
@beckjake -- sounds good! As I run into any other issues I'll keep that in mind. |
|
At this point, it looks like the unit tests are passing, but integration tests are failing (apparently due to set up issues). I ran the unit tests locally against networkx 1.11, and they pass with that version as well. |
There was a problem hiding this comment.
This looks great! Non-postgres tests run on hosted cloud services that have real credentials, so they require manual review from us before they can run, hence the permissions errors.
There's one one unit test change if you would be so kind, then I'll kick off a full CI build and we can get this merged.
I'm so glad we're moving to networkx 2, thanks for your contribution!
There was a problem hiding this comment.
This looks great, thanks @heisencoder - I've triggered the tests, and once they pass I'll merge this in for 0.14.0!
|
The tests did pass, I'm not sure why but github is linking the wrong result: this is correct |
|
Thanks for your contribution to dbt @heisencoder! |
Changes to make linker.py work with both networkx version 1 and 2.
See #1496