Task EvolvePokemon should give us a status updates so we know it is working #5570

ch1ago · 2016-09-20T16:58:53Z

Hey guys,
I have done some local changes on this task so I know how far it is to start evolving my pokemon.

I really like the changes locally and I think they would be good for everybody.
Could anybody take a look and show me how to reduce the logging throughput.

Thanks ❤️

mention-bot · 2016-09-20T16:58:54Z

@thejamespinto, thanks for your PR! By analyzing the annotation information on this pull request, we identified @achretien, @alexyaoyang and @novadev94 to be potential reviewers

alexyaoyang · 2016-09-20T17:05:15Z

@thejamespinto Thanks for the PR! I can see that it will be very helpful! 👍

There's just one small question, how often does it show the status update? If it's too quick (once every second), we gotta limit it so it isn't too spammy.

DBa2016

While I don't see this as particularly useful, there is no harm in it either.

ch1ago · 2016-09-20T18:11:37Z

this is my first PR and I don't know how to filter the log throughput.
I used a sleep(60) locally.
What do you guys suggest?

ghost · 2016-09-20T19:32:41Z

Should be a configurable "min_interval", imho.

alexyaoyang · 2016-09-20T21:14:15Z

@thejamespinto As @acurioustale suggested or you could try using "log_interval" too :)

Gobberwart · 2016-09-20T23:43:34Z

@thejamespinto using sleep(60) will halt bot execution completely for 60 seconds. We definitely don't want that.

Gobberwart · 2016-09-20T23:46:09Z

pokemongo_bot/cell_workers/evolve_pokemon.py

@@ -66,12 +66,15 @@ def work(self):
                pokemon_to_be_evolved = pokemon_to_be_evolved + min(candy.quantity / (pokemon.evolution_cost - 1), filtered_dict[pokemon.pokemon_id])

        if pokemon_to_be_evolved >= self.min_pokemon_to_be_evolved:
+            self.logger.info("Checking minimum pokemon to evolve: {}/{}. Hooray!".format(pokemon_to_be_evolved, self.min_pokemon_to_be_evolved))


Do we REALLY need "Hooray!"?

Also... it's not "checking". It has checked.

On second thought, do we even need this? It will immediately start evolving so probably no need for another alert.

I just think it's valid to state two versions of the same message so people know if something's wrong

Gobberwart · 2016-09-20T23:46:49Z

pokemongo_bot/cell_workers/evolve_pokemon.py

            if self.use_lucky_egg:
                self._use_lucky_egg()
            cache = {}
            for pokemon in filtered_list:
                if pokemon.can_evolve_now():
                    self._execute_pokemon_evolve(pokemon, cache)
+        else:
+            self.logger.info("Checking minimum pokemon to evolve: {}/{}. Not enough, gotta catch`em all (over and over again)".format(pokemon_to_be_evolved, self.min_pokemon_to_be_evolved))


Again, do we need humorous status messages? Not a fan.

Suggest "Not enough pokemon for evolve" would do.

alexyaoyang · 2016-09-21T03:50:14Z

@Gobberwart I agree with you but chill man, cut him some slack, it's his first time contributing to us. 👍

@thejamespinto For examples of limiting throughput, take a look at base_task.py for log_interval or update_live_stats.py for min_interval.

Give some revision to the messages (this bot does lean towards more simplistic logging) + have the throughput reduced and I'll approve your PR 👍

PS: be careful when reducing the throughput so that it doesn't affect the frequency of the pokemon_evolved emit event too, we definitely want all the evolve messages to show :)

ch1ago · 2016-09-22T16:34:02Z

@alexyaoyang yeah, thanks for holding my back against that awful bully 😢

JK, we actually interact quite a lot on Slack and I had personally asked him to come and review this PR. 😄

I have made some deeper changes and I'll add the changes in a few 👍
I'm creating a new event named pokemon_evolve_check

…orking

ch1ago · 2016-09-22T16:56:48Z

@alexyaoyang I've re-patched it.

I wish my python skills were better so I could help out this project further with tests and such.

I have learned this tasks runs repeatedly every second, this extra config will make it so it only runs once every 60 seconds by default, so it doesn't flood the output.

I hope these changes turn out valuable to the community.

Botta catch`em all!

alexyaoyang

@thejamespinto Great job on the new patch! Left a few comments below. 👍

alexyaoyang · 2016-09-22T17:26:24Z

pokemongo_bot/cell_workers/evolve_pokemon.py

+        result_message = ("Gotta catch`em all!", "Gotta evolv`em all!")[has_minimum_to_evolve]
+        self.emit_event(
+            'pokemon_evolve_check',
+            formatted='Checking... Has {has}, needs {needs}. {message}',


I think it can be even more simplistic: Evolvable: {has}/{need} will be sufficient. 👍

alexyaoyang · 2016-09-22T17:31:47Z

pokemongo_bot/cell_workers/evolve_pokemon.py

    def _should_run(self):
        if not self.evolve_list or self.evolve_list[0] == 'none':
            return False
-        return True
-
+        return self.next_update is None or datetime.now() >= self.next_update


It seems like the duration that user set now affects how frequent evolve pokemon task is ran. I think only the logging should be reduced, but not the task itself.

For example, if I set min_pokemon_to_be_evolved to 80, there's a chance that I might be evolving more than 80 pokemon due to evolve pokemon not running when I have exactly 80 pokemon.

While evolving more pokemon than the quota is not a big deal for me, I'm not sure if that applies to everyone else. Also if user accidentally sets interval to a huge number, that might be a bigger issue.

I'm sorry, but I added documentation for this and the way I see this, people should read it so as not to put a huge number there. Also, how many extra pokemon can they catch in 120 seconds?

Having said that, I can still tweak this to a log_interval if you want ¯_(ツ)_/¯

Hmmm yes I see what you mean and I agree with you, but I'd rather not change the behavior of the task since the purpose of this PR is for giving a status update, and not to reduce frequency of evolve task.

Assuming that not all users read the documentation, we are essentially giving users the power/ability to screw up the evolve task.

IMO, if the frequency of evolve task should be reduced, it should be in another PR. What do you think?

@alexyaoyang Hey man, chill. It's his first contribution :) :) :)

Just kidding, I agree with you. Keeping this PR to just the log message, not the task itself, would be better.

@Gobberwart Not sure why but I found that very funny. Now people looking at me kinda weird in Starbucks. Thanks!

alexyaoyang · 2016-09-22T17:43:02Z

configs/config.json.example

@@ -153,6 +153,7 @@
        "type": "EvolvePokemon",
        "config": {
          "enabled": false,
+          "interval": 60,


Small issue, but I'm not sure if users will be confused by interval and min_evolve_speed. Perhaps a better name is log_interval?

this number represents how often - in seconds - the task runs. I just realized I should also add that to the docs.

@alexyaoyang I've added the documentation line. I have been trying this at home for 2 days now and I feel much more comfortable with 120 as the default value because we don't want this message to be spammy 😄

…orking

alexyaoyang · 2016-09-23T03:23:57Z

@thejamespinto I think you missed a few of my comments :)

ch1ago · 2016-09-23T12:12:55Z

@alexyaoyang I did, sorry, I got caught up with the very last comment and I thought it was your only review :)

…orking

alexyaoyang · 2016-09-26T01:55:49Z

@thejamespinto Still waiting on your changes :)

ch1ago · 2016-09-26T03:44:19Z

@alexyaoyang I'm gonna release them, just kind of caught up with life and work. :)

ch1ago · 2016-09-26T04:08:57Z

@alexyaoyang there we go 😄

…orking

alexyaoyang · 2016-09-26T05:52:33Z

pokemongo_bot/cell_workers/evolve_pokemon.py

+                'pokemon_evolve_check',
+                formatted='Evolvable: {has}/{need}',
+                data={'has': has, 'needs': needs}
+            )


One last question, will self._should_log_update always return false with self._compute_next_log_update above?

no, it may return either True or False,
but I have just realized I made it impossible for self.next_log_update to be None. Nice catch. 👍

…orking

alexyaoyang · 2016-09-26T17:58:09Z

👍 LGTM

DBa2016 approved these changes Sep 20, 2016

View reviewed changes

Gobberwart reviewed Sep 20, 2016

View reviewed changes

alexyaoyang added the Waiting on Changes label Sep 21, 2016

Task EvolvePokemon should give us a status updates so we know it is w…

f76b84a

…orking

ch1ago force-pushed the patch-2 branch from 988fe98 to f76b84a Compare September 22, 2016 16:49

alexyaoyang reviewed Sep 22, 2016

View reviewed changes

ch1ago changed the title ~~[WIP] Task EvolvePokemon should give us a status updates so we know it is working~~ Task EvolvePokemon should give us a status updates so we know it is working Sep 22, 2016

Task EvolvePokemon should give us a status updates so we know it is w…

f45267f

…orking

Task EvolvePokemon should give us a status updates so we know it is w…

2fa4fda

…orking

Task EvolvePokemon should give us a status updates so we know it is w…

c8036a4

…orking

ch1ago force-pushed the patch-2 branch from f0d4e8a to c8036a4 Compare September 26, 2016 04:25

alexyaoyang reviewed Sep 26, 2016

View reviewed changes

Task EvolvePokemon should give us a status updates so we know it is w…

2b10ee4

…orking

alexyaoyang merged commit c53a57e into PokemonGoF:dev Sep 26, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task EvolvePokemon should give us a status updates so we know it is working #5570

Task EvolvePokemon should give us a status updates so we know it is working #5570

ch1ago commented Sep 20, 2016

mention-bot commented Sep 20, 2016

alexyaoyang commented Sep 20, 2016

DBa2016 left a comment

ch1ago commented Sep 20, 2016 •

edited

Loading

ghost commented Sep 20, 2016 •

edited by ghost

Loading

alexyaoyang commented Sep 20, 2016

Gobberwart commented Sep 20, 2016

Gobberwart Sep 20, 2016

Gobberwart Sep 21, 2016

ch1ago Sep 22, 2016

Gobberwart Sep 20, 2016

alexyaoyang commented Sep 21, 2016

ch1ago commented Sep 22, 2016 •

edited

Loading

ch1ago commented Sep 22, 2016 •

edited

Loading

alexyaoyang left a comment

alexyaoyang Sep 22, 2016 •

edited

Loading

alexyaoyang Sep 22, 2016 •

edited

Loading

ch1ago Sep 23, 2016 •

edited

Loading

alexyaoyang Sep 23, 2016

Gobberwart Sep 23, 2016

alexyaoyang Sep 23, 2016

alexyaoyang Sep 22, 2016

ch1ago Sep 22, 2016

ch1ago Sep 23, 2016

alexyaoyang commented Sep 23, 2016

ch1ago commented Sep 23, 2016

alexyaoyang commented Sep 26, 2016

ch1ago commented Sep 26, 2016

ch1ago commented Sep 26, 2016

alexyaoyang Sep 26, 2016

ch1ago Sep 26, 2016

alexyaoyang commented Sep 26, 2016 •

edited by douglascamata

Loading

Task EvolvePokemon should give us a status updates so we know it is working #5570

Task EvolvePokemon should give us a status updates so we know it is working #5570

Conversation

ch1ago commented Sep 20, 2016

mention-bot commented Sep 20, 2016

alexyaoyang commented Sep 20, 2016

DBa2016 left a comment

Choose a reason for hiding this comment

ch1ago commented Sep 20, 2016 • edited Loading

ghost commented Sep 20, 2016 • edited by ghost Loading

alexyaoyang commented Sep 20, 2016

Gobberwart commented Sep 20, 2016

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

alexyaoyang commented Sep 21, 2016

ch1ago commented Sep 22, 2016 • edited Loading

ch1ago commented Sep 22, 2016 • edited Loading

alexyaoyang left a comment

Choose a reason for hiding this comment

alexyaoyang Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

alexyaoyang Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

ch1ago Sep 23, 2016 • edited Loading

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

alexyaoyang commented Sep 23, 2016

ch1ago commented Sep 23, 2016

alexyaoyang commented Sep 26, 2016

ch1ago commented Sep 26, 2016

ch1ago commented Sep 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexyaoyang commented Sep 26, 2016 • edited by douglascamata Loading

ch1ago commented Sep 20, 2016 •

edited

Loading

ghost commented Sep 20, 2016 •

edited by ghost

Loading

ch1ago commented Sep 22, 2016 •

edited

Loading

ch1ago commented Sep 22, 2016 •

edited

Loading

alexyaoyang Sep 22, 2016 •

edited

Loading

alexyaoyang Sep 22, 2016 •

edited

Loading

ch1ago Sep 23, 2016 •

edited

Loading

alexyaoyang commented Sep 26, 2016 •

edited by douglascamata

Loading