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

Chore/py3 #119

Merged
merged 28 commits into from
Feb 5, 2020
Merged

Chore/py3 #119

merged 28 commits into from
Feb 5, 2020

Conversation

vpsx
Copy link
Contributor

@vpsx vpsx commented Dec 24, 2019

Update to python 3 yayayayay!!!

Regarding node level bug (see bug fixes section), see PXP-5067

Breaking Changes

  • python 3!

Bug Fixes

  • in test, guard case where right_val is list but left_val is None (should not attempt to sort left_val)
  • propose fix for bug where node levels of leaf nodes are set to strings, in particular string reprs of edge table names

Improvements

  • sort dependencies lexicographically
  • use loose pins in setup.py
  • rename confusing varname items_equal to items_not_equal
  • initialize log level in the parent logger per cdislogging 1.0.0 breaking change
  • promote idiomatic python by changing "type(v) is t" to "isinstance(v,t)"

Dependency updates

  • require python 3
  • bump dictionaryutils, gen3datamodel (fka gdcdatamodel), psqlgraph, psycopg2, pyspark, six
  • remove indirect dependencies oauthlib, psutil, python_dateutil, pyyaml

@@ -123,10 +123,22 @@ def update_level(self):
new_assigned = set([])
for collector in just_assigned:
for child in collector.children:
if len(child.children) == 0 or child in assigned_levels:
# TODO: PXP-5067 investigate int-string comparison in node lvls
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand your comment here. Why do we need to change the logic here? what is the bug?

@vpsx
Copy link
Contributor Author

vpsx commented Jan 6, 2020

@thanh-nguyen-dang, in response to your comments here and elsewhere, here is how you can reproduce the problem at hand:

  1. Check out current Tube master branch (0.3.22)
  2. Set up test database with tube/tests/metadata_db.sql; set up ES, run ETL
  3. Add the following code to tube/etl/indexers/injection/nodes/collecting_node.py, in the __lt__ method of CollectingNode, before the return:
if isinstance(self.level, str) or isinstance(other.level, str):
    import pdb; pdb.set_trace()
  1. Run unit tests to see if breakpoint is hit. Behold.
  2. Inspect the values: type(self.level), type(other.level), type(self), type(other). You see that both are collecting nodes and that there occur several LT comparisons of int and string.
  3. Observe that whereas in python 2 this Just Works, in python 3 this raises a type error.

As I said in the ticket, perhaps this is deliberate, but it is pretty idiosyncratic, and at any rate python 3 won't allow it.

m0nhawk
m0nhawk previously approved these changes Feb 4, 2020
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.

3 participants