Update atomtyper.py to use networkX 2.X methods

[justinGilmer]

PR Summary:

Upgrading to networkX >= 2.4 removes the nx.Graph.node method, breaking
atomtyper.py.

NetworkX recently updated to 2.4 and the way we access nodes have been
removed: G.node –> use G.nodes. This was only present in atomtyper.py,
the rest of our node calls were using the new syntax.

Updating to handle the new syntax restores expected behavior, and all
unit tests pass.

PR Checklist

---

• Includes appropriate unit test(s)
• Appropriate docstring(s) are added/updated
• Code is (approximately) PEP8 compliant
• Issue(s) raised/addressed?

Addresses issue #279

[mikemhenry]

Since we didn't pin the last version, we should probably consider making this a hotfix and re-doing the release since anyone downloading the current version off of conda will run into problems... I think...

[justinGilmer]

I agree, any updates and new installs will not work with our current releases

[mikemhenry]

This is the second time something broke with networkx, I vote we pin to the current version but allow minor versions, so like 2.4.X

[justinGilmer]

I am not sure if that is necessary. We already pin to >= 2.0, which had the graph.nodes method. The issue was we were using a deprecated method from 1.X. What benefit would pinning the version bring?

[mikemhenry]

It would mean that if we had pined to to <2.4, things wouldn't have broken. We should be pining most of our stuff like that IMHO since we don't want a change upstream breaking a release. When we are ready to do a release we could relax the pinning, see what versions are working, then re-pin on those to keep things updated.

[codecov]

Codecov Report

Merging #280 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #280   +/-   ##
=======================================
  Coverage   86.17%   86.17%           
=======================================
  Files          11       11           
  Lines        1179     1179           
=======================================
  Hits         1016     1016           
  Misses        163      163

[justinGilmer]

Ok, I think I might have miss-represented what happened here. nx.graph.nodes has been available since version 2.0, and works for all versions of NX >=2.0. nx.node was deprecated since 2.0, we just never made the change in syntax. So this update should be backwards compatible with all versions of 2.X.

The big breaking changes were fixed when pinning to >=2.0, we just ignored the deprecation warning it provided. maybe pinning NX >=2.0 <=3.0 might be more correct?

[mikemhenry]

That sounds reasonable, it was our fault for not updating it before it was broken... We should consider running CI with fail on warn or something to force us to fix? Either way I'm happy with pinning it the way you described.

[uppittu11]

mBuild and Foyer tests are passing with the latest networkx

[mattwthompson]

I'm of two minds on pinning; in general I think over-pinning is something we want to avoid, but I don't see how we can insulate ourselves against breaking upstream changes unless we catch issues like this and push out hotfix releases on a short lead time (like a day). In this case, since this new syntax works on both new and older versions, I think this is a good change. But we should have a larger discussion about pinning and how to manage our deps.

[mattwthompson]

This change LGTM, not sure about pinning <3.0 (at least in this PR).

[mikemhenry]

I don't mind being overruled in this situation since its 2v1 right now, but it looks like 2.5 isn't even out yet, so pinning less than version 3 should be fine since the current version is 2.4

[mattwthompson]

You did bring up a good point when you suggested when push out a hotfix for this change. This seems necessarily and fairly urgent, no?

[mikemhenry]

Yes, anyone who installs foyer will run into an issue. Once we get this merged people who update from source will be okay, but until we re-release, it will be broken for people. All previous versions that didn't have the pin to the right networkx version will stay broken unless a user downgrades to the right version of networkx.

[uppittu11]

I couldn't find any networkx deprecation warnings in the CI build logs for mbuild or foyer. I'm also not getting any warnings if I use Graph.node using networkx=2.3 locally. I'm not sure how we could have caught this issue before now.

[mikemhenry]

python -c "import warnings; print(warnings.filters)" will show current filters:

[('ignore', None, <class 'DeprecationWarning'>, None, 0), 
('ignore', None, <class 'PendingDeprecationWarning'>, None, 0), 
('ignore', None, <class 'ImportWarning'>, None, 0), 
('ignore', None, <class 'BytesWarning'>, None, 0), 
('ignore', None, <class 'ResourceWarning'>, None, 0)]

So unless you use something like python -Wall you won't see them

[mattwthompson]

Not sure if this is simple but it would be nice to have some analytics from AZP about warnings, if that can be parsed through pytest to the web interface. Maybe there's a plugin for that?