Migrator hash #197

CJ-Wright · 2018-06-20T23:16:48Z

Closes #189
closes #138

codecov-io · 2018-06-20T23:19:50Z

Codecov Report

Merging #197 into master will increase coverage by 1.25%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #197      +/-   ##
=========================================
+ Coverage   13.35%   14.6%   +1.25%     
=========================================
  Files           7       7              
  Lines         397     397              
=========================================
+ Hits           53      58       +5     
+ Misses        344     339       -5

Impacted Files	Coverage Δ
conda_forge_tick/update_upstream_versions.py	`0% <ø> (ø)`	⬆️
conda_forge_tick/make_graph.py	`0% <ø> (ø)`	⬆️
conda_forge_tick/utils.py	`86.36% <100%> (+22.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81036cb...8c206c0. Read the comment docs.

justcalamari · 2018-06-21T01:08:23Z

conda_forge_tick/auto_tick.xsh

    # If we've gotten this far then the node is good
    attrs['bad'] = False
    print('Removing feedstock dir')
    rm -rf @(feedstock_dir)
-    return True
+    return migrate_return


Won't migrate_return always be True if we get here?

Ah, yes I forgot to do the upate in main.

CJ-Wright · 2018-06-21T01:11:52Z

@justcalamari @scopatz ready for review! This PR will make sure that we don't re-run migrations that we have already run.

isuruf · 2018-06-21T01:13:01Z

conda_forge_tick/auto_tick.xsh

@@ -15,7 +15,9 @@ from .git_utils import (get_repo, push_repo)
 # TODO: move this back to the bot file as soon as the source issue is sorted
 # https://travis-ci.org/regro/00-find-feedstocks/jobs/388387895#L1870
 from .migrators import *
-$MIGRATORS = [Version(), Compiler()]
+$MIGRATORS = [Version(), 
+# Compiler()


Compiler migration is not complete though

right, isn't that why it is commented out?

Correct, I need to manually migrate the graph or we'll issue PRs to all outstanding compiler migrations. This issue was one of the driving factors for making these changes.

scopatz · 2018-06-21T01:21:33Z

conda_forge_tick/auto_tick.xsh

+                  migrator.pr_head(), migrator.remote_branch())
+    except github3.GitHubError as e:
+        if e.msg != 'Validation Failed':
+            raise e


This should just be a reraise, ie raise not raise e

scopatz · 2018-06-21T01:21:56Z

conda_forge_tick/auto_tick.xsh

+        if e.msg != 'Validation Failed':
+            raise e
+        else:
+            pass


else: pass is a no-op, that you can remove.

re-raise properly pass migrator_hash to PRed set

justcalamari · 2018-06-21T01:25:38Z

conda_forge_tick/migrators.xsh

+        return f'{n}_{attrs["new_version"]}'
+
+    def _extract_version_from_hash(self, h):
+        return h.split('_')[-1]


What happens when the version has an underscore in it?

I'm not sure if conda allows that but I will put in some safe guards.

conda does allow '_'

justcalamari · 2018-06-21T01:36:57Z

conda_forge_tick/auto_tick.xsh

-                                       'smithy_version': smithy_version,
+                if 'PRed' not in gx.nodes[node]:
+                    gx.nodes[node]['PRed'] = set()
+                gx.nodes[node]['PRed'].add(migrator_hash)


Maybe do gx.nodes[node].setdefault('PRed', set()).add(migrator_hash) here to reduce lines.

justcalamari · 2018-06-21T01:41:19Z

conda_forge_tick/migrators.xsh

+            or (VersionOrder(str(attrs['new_version'])) <=
+                VersionOrder(str(attrs['version'])))
+            # if PRed version is greater than newest version
+            or any(VersionOrder(self._extract_version_from_hash(h)) >=


I think sometimes h will be False. Then _extract_version_from_hash will error.

right, so now we default to '0.0.0'

justcalamari · 2018-06-21T01:52:08Z

tests/test_migrators.py

+    pmy['meta_yaml'] = parse_meta_yaml(inp)
+    pmy['raw_meta_yaml'] = inp
+    pmy.update(kwargs)
+
    m = migrator()


Can we use a single instance of each migrator for all tests? This will make the tests more robust.

justcalamari · 2018-06-21T01:54:41Z

tests/test_migrators.py

    (Version, multi_source, updated_multi_source, {'new_version': '2.4.1'},
-     'Please check that the dependencies have not changed.'),
-    (Version, sample_r, updated_sample_r, {'new_version': '1.3_2'},


Why was 1.3_2 changed to 1.3-2?

CJ-Wright · 2018-06-21T02:46:23Z

@justcalamari @scopatz @isuruf Thank you for the review! Would it be possible to get another round?

CJ-Wright · 2018-06-21T15:48:10Z

conda_forge_tick/migrators.xsh

@@ -89,6 +93,9 @@ class Migrator:
        """Branch to use on local and remote"""
        return 'bot-pr'

+    def migrator_hash(self, attrs):
+        return f'{self.__class__.__name__}_{self._class_version}'


I'm trying to remember why we didn't go with a tuple for this. A tuple might make life easier.

Or maybe a dict?

You can always change it :)

Ah, that is why, I can't put dicts into sets. I can put tuples though.

But maybe I can live with lists since dicts seem nicer.

A namedtuple might be what you want, actually

Will namedtuple allow me to add keys as needed or do I need to put out a new one for each set of keys?

justcalamari · 2018-06-21T16:19:11Z

conda_forge_tick/migrators.xsh

+        # don't run on bad nodes
+        return (bool(attrs.get('archived', False)
+                or self.migrator_hash(attrs) in attrs.get('PRed', []))
+                or attrs.get('bad', False))


The problem with this is that the graph thinks that good nodes that used to be bad are still bad. We would need to set bad to False somewhere earlier in the code.

Hmm ok, should we reset bad here? https://github.com/regro/cf-scripts/blob/master/conda_forge_tick/make_graph.py#L15
That way every node is good at least to start with.
Or would this introduce too much of a burden?

I think that should be fine.

We may need to think about this more, since failing on every bad feedstock every time the bot runs could be a problem.

justcalamari · 2018-06-21T16:42:03Z

conda_forge_tick/migrators.xsh

@@ -89,6 +95,10 @@ class Migrator:
        """Branch to use on local and remote"""
        return 'bot-pr'

+    def migrator_hash(self, attrs):
+        return {'class': self.__class__.__name__,
+                'class_version': self._class_version}


Making this a dict complicates searching with whoosh. I can probably write a new whoosh field type for list of dict, but we need to decide what key names to use. Or maybe we don't care about searching PRed and it can be left out of the schema.

Hmm, how are we going to handle multi-source packages, don't they have a list of dicts?
I wouldn't worry about searching this info yet. This will most likely need to go through a refactor when libcflib has a feedstocks graph.

Multi-source packages have a list of dicts for what field?

source https://conda.io/docs/user-guide/tasks/build-packages/define-metadata.html#source-from-multiple-sources

return pull request json object for later use (eg close PRs if they are old, send pings to maintainers when the status checks are finished)

justcalamari · 2018-06-21T16:47:08Z

conda_forge_tick/auto_tick.xsh

-                # TODO: capture pinning here too!
-                gx.nodes[node].update({'PRed': attrs['new_version'],
-                                       'smithy_version': smithy_version,
+                migrator_hash = run(attrs=attrs, migrator=migrator, gh=gh,


Run also returns PR json now.

justcalamari · 2018-06-21T17:06:03Z

conda_forge_tick/auto_tick.xsh

+                gx.nodes[node].setdefault('PRed', []).append(migrator_hash)
+                # Stash the pr json data so we can access it later
+                gx.nodes[node].setdefault('PRed_json', {}).update(
+                    {tuple(migrator_hash.keys()): pr_json})


This seems like a weird key. Did you mean migrator_hash.values()?

add migration script for moving the graph to the new PRed format

justcalamari · 2018-06-21T17:28:49Z

migrate_to_hash.py

+    compiler_migrations.append(node)
+
+last_compiler_pr = 'cxxopts'
+last_compiler_index = compiler_migrations.index(last_compiler_pr)


Is compiler_migrations in the same order every time?

I think so (based off of the last time we hit the exact same recipes). I think it is alphabetical.

CJ-Wright · 2018-06-21T17:53:38Z

conda_forge_tick/auto_tick.xsh

+                                    hash_type=attrs.get('hash_type', 'sha256'))
+
+                converted_hash = convert_dict_to_nt(migrator_hash)
+                gx.nodes[node].setdefault('PRed', set()).update(converted_hash)


add not update

CJ-Wright · 2018-06-21T17:58:55Z

conda_forge_tick/migrators.xsh

+            # if PRed version is greater than newest version
+            or any(VersionOrder(self._extract_version_from_hash(h)) >=
+                   VersionOrder(attrs['new_version']
+                                ) for h in attrs.get('PRed', [])))


CJ-Wright · 2018-06-21T17:59:03Z

conda_forge_tick/migrators.xsh

+        return n
+
+    def _extract_version_from_hash(self, h):
+        return h.get('version', '0.0.0')


needs getattr

CJ-Wright · 2018-06-21T18:05:45Z

@justcalamari @scopatz @isuruf one last round?

Once this is merged, I'll:

migrate the graph manually
check the graph
commit/push the graph
Manually trigger a PRing round (without the compiler)
If everything works then I'll put the compiler migration back in and run another round and monitor it for any issues.

I think after that we should be back to normal.

CJ-Wright · 2018-06-21T18:09:10Z

@mariusvniekerk (would you be interested in giving review?)

justcalamari · 2018-06-21T18:25:05Z

conda_forge_tick/migrators.xsh

-        return bool(attrs.get('archived', False))
+        # don't run on things we've already done
+        # don't run on bad nodes
+        return bool(bool(attrs.get('archived', False)


Is the bool casting needed?

I was going to play it safe

justcalamari · 2018-06-21T18:29:39Z

conda_forge_tick/auto_tick.xsh

+                                    rerender=rerender, protocol='https',
+                                    hash_type=attrs.get('hash_type', 'sha256'))
+
+                converted_hash = convert_dict_to_nt(migrator_hash)


Why not have migrator_hash be a namedtuple to begin with?

scopatz · 2018-06-21T18:50:59Z

conda_forge_tick/migrators.xsh



 class Migrator:
    """Base class for Migrators"""
    rerender = False

+    _class_version = 0


I would prefer if this was called migrator_version, and it seems like this should be part of the public API

scopatz · 2018-06-21T18:54:07Z

conda_forge_tick/migrators.xsh

@@ -89,6 +96,10 @@ class Migrator:
        """Branch to use on local and remote"""
        return 'bot-pr'

+    def migrator_hash(self, attrs):


docstring on this method. Also, it doesn't seem to return a hash (int)

scopatz · 2018-06-21T18:54:41Z

conda_forge_tick/migrators.xsh

@@ -89,6 +96,10 @@ class Migrator:
        """Branch to use on local and remote"""
        return 'bot-pr'

+    def migrator_hash(self, attrs):
+        return {'class_name': self.__class__.__name__,
+                'class_version': self._class_version}


I think you can drop the class_ from these keys.

Or replace them with migrator_, which has more semantic meaning.

scopatz · 2018-06-21T18:55:27Z

conda_forge_tick/migrators.xsh

@@ -240,6 +257,15 @@ class Version(Migrator):
    def remote_branch(self):
        return self.attrs['new_version']

+    def migrator_hash(self, attrs):
+        n = super().migrator_hash(attrs)
+        n.update({'version': attrs["new_version"]})


Oh, I see that this adds version

scopatz · 2018-06-21T18:57:03Z

migrate_to_hash.py

@@ -0,0 +1,29 @@
+import networkx as nx


This should probably have a #!/usr/bin/env python as a script

justcalamari · 2018-06-21T19:57:32Z

conda_forge_tick/auto_tick.xsh

+                                    rerender=rerender, protocol='https',
+                                    hash_type=attrs.get('hash_type', 'sha256'))
+
+                converted_hash = migrator_uid


I think this line can be removed and every instance of converted_hash changed to migrator_uid

justcalamari · 2018-06-21T20:00:26Z

conda_forge_tick/migrators.xsh

@@ -44,7 +51,7 @@ class Migrator:
        bool:


The return type is different now

justcalamari · 2018-06-21T20:00:55Z

conda_forge_tick/auto_tick.xsh

+
+    Returns
+    -------
+    migrate_return: dict


type should be namedtuple

don't write to exceptions file, just put it into the graph style changes

CJ-Wright · 2018-06-21T20:36:57Z

Interesting, trying to pickle the namedtuple is causing problems:

christopher@christopher-ubuntu ~/dev/cf-graph master $ python ../cf-scripts/migrate_to_hash.py 
Traceback (most recent call last):
  File "../cf-scripts/migrate_to_hash.py", line 30, in <module>
    nx.write_gpickle(g, '../cf-graph/graph.pkl')
  File "<decorator-gen-402>", line 2, in write_gpickle
  File "/home/christopher/mc/lib/python3.6/site-packages/networkx/utils/decorators.py", line 227, in _open_file
    result = func_to_be_decorated(*new_args, **kwargs)
  File "/home/christopher/mc/lib/python3.6/site-packages/networkx/readwrite/gpickle.py", line 70, in write_gpickle
    pickle.dump(G, path, protocol)
_pickle.PicklingError: Can't pickle <class 'conda_forge_tick.utils.converted'>: attribute lookup converted on conda_forge_tick.utils failed

time with pickle

CJ-Wright · 2018-06-21T21:16:15Z

I think this is ready to go! I've tested this on the graph and it seems to produce the expected output.
Any last issues?
If not I'd like to have this merged by 6pm EST so I can start the PRing and do any troubleshooting.

Thank you everyone for the reviews! (sorry that this PR was a bit like pulling chicken teeth)

CJ-Wright added 2 commits June 20, 2018 17:30

don't do new compilers twice

9ce65c4

wip migrator hash

a515ded

fix compiler string for test

ceedd0b

justcalamari reviewed Jun 21, 2018

View reviewed changes

ENH: don't re-run migrations

0fd9937

isuruf reviewed Jun 21, 2018

View reviewed changes

scopatz reviewed Jun 21, 2018

View reviewed changes

remove no-op

515f9c1

re-raise properly pass migrator_hash to PRed set

justcalamari reviewed Jun 21, 2018

View reviewed changes

safeguard against _ versions

c98ab80

justcalamari reviewed Jun 21, 2018

View reviewed changes

comments from @justcalamari

3559ac6

black

411236f

CJ-Wright mentioned this pull request Jun 21, 2018

Issue PRs changing toolchain over to jinja2 compiler syntax #162

Closed

CJ-Wright commented Jun 21, 2018

View reviewed changes

CJ-Wright added 3 commits June 21, 2018 12:00

use dict hashes for migrators

4e83c86

exclude "bad" feedstocks from migration

ba65ad5

add TODO about namedtuple

3112070

justcalamari reviewed Jun 21, 2018

View reviewed changes

make_graph overwrites initial bad setting

75f2511

return pull request json object for later use (eg close PRs if they are old, send pings to maintainers when the status checks are finished)

justcalamari reviewed Jun 21, 2018

View reviewed changes

stash the pr json into the graph attrs so we can track the PR

f716219

justcalamari reviewed Jun 21, 2018

View reviewed changes

CJ-Wright added 2 commits June 21, 2018 13:15

use correct key for PR json

e530587

add migration script for moving the graph to the new PRed format

write graph back to disk

c25fe5d

justcalamari reviewed Jun 21, 2018

View reviewed changes

CJ-Wright added 3 commits June 21, 2018 13:48

use converted named tuples for PRed

02b67fe

use converted named tuples for PRed

e2da3ed

use set/namedtuple syntax

239f564

CJ-Wright commented Jun 21, 2018

View reviewed changes

justcalamari reviewed Jun 21, 2018

View reviewed changes

scopatz reviewed Jun 21, 2018

View reviewed changes

migrate_to_hash.py

@@ -0,0 +1,29 @@

import networkx as nx

Copy link

Contributor

scopatz Jun 21, 2018

Choose a reason for hiding this comment

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

This should probably have a #!/usr/bin/env python as a script

CJ-Wright added 3 commits June 21, 2018 15:41

change migrator uids and put out namedtuples

d7b48af

change migrator uids for script

a444dc5

remove convert since we have uid in namedtuple format already

3da9113

justcalamari reviewed Jun 21, 2018

View reviewed changes

conda_forge_tick/migrators.xsh Outdated

@@ -44,7 +51,7 @@ class Migrator:

bool:

Copy link

Contributor

justcalamari Jun 21, 2018

Choose a reason for hiding this comment

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

The return type is different now

justcalamari reviewed Jun 21, 2018

View reviewed changes

conda_forge_tick/auto_tick.xsh Outdated

Returns

-------

migrate_return: dict

Copy link

Contributor

justcalamari Jun 21, 2018

Choose a reason for hiding this comment

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

type should be namedtuple

CJ-Wright added 2 commits June 21, 2018 16:07

fix comments from @justcalamari

8bd8ccf

don't override pr_json if we just push to our branch.

ad50b30

don't write to exceptions file, just put it into the graph style changes

move back to dicts and lists since namedtuple was giving us a hard

8c206c0

time with pickle

CJ-Wright merged commit 8619de4 into regro:master Jun 21, 2018

CJ-Wright deleted the migrator_hash branch June 21, 2018 22:20

Migrator hash #197

Migrator hash #197

Conversation

CJ-Wright commented Jun 20, 2018 • edited

codecov-io commented Jun 20, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CJ-Wright commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CJ-Wright Jun 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcalamari Jun 21, 2018 • edited

Choose a reason for hiding this comment

CJ-Wright commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CJ-Wright commented Jun 21, 2018

CJ-Wright commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CJ-Wright commented Jun 21, 2018

CJ-Wright commented Jun 21, 2018

CJ-Wright commented Jun 20, 2018 •

edited

codecov-io commented Jun 20, 2018 •

edited

CJ-Wright Jun 21, 2018 •

edited

justcalamari Jun 21, 2018 •

edited