Rudimentary q table #42

MattSkala · 2018-12-03T13:44:20Z

No description provided.

synctext · 2018-12-10T11:02:37Z

plebnet/agent/qtable.py

+                self.providers_offers = data['providers_offers']
+                self.self_state = data['self_state']
+
+    def choose_best_option(self, providers):


Seems very "hard coded"

I moved values to the single constant to have less "hard coded code"

rpytel1 · 2018-12-11T16:51:44Z

@Jaapp- I think smth failed here with Jenkins(or integration to git), because I think all the tests pass now and so it looks on the Jenkins.
@andrebreis @MattSkala @BloodyFool feel free to comment the code and I can repair it before merging.

rpytel1 · 2018-12-11T17:04:33Z

plebnet/agent/core.py

@@ -34,21 +35,28 @@ def setup(args):
    agent. All necessary configuration files are created and IRC communication is started.
    :param args: If running in Testnet mode.
    """
-    global dna, config
+    global dna, qtable, config


rpytel1 · 2018-12-11T17:05:01Z

plebnet/agent/core.py

@@ -13,6 +13,8 @@
 import subprocess
 import shutil

+from plebnet.agent.qtable import QTable
+
 from plebnet.agent.dna import DNA


remove unused imports

rpytel1 · 2018-12-11T17:06:33Z

plebnet/agent/core.py

@@ -218,6 +221,8 @@ def attempt_purchase():
    """
    Check if enough money to buy a server, and if so, do so,
    """
+    config = PlebNetConfig()
+    qtable = QTable()


check if it is crucial

rpytel1 · 2018-12-11T17:13:39Z

plebnet/agent/qtable.py

+        """
+        Creates the QTable configuration for the child agent. This is done by copying the own QTable configuration and including the new host provider, the parent name and the transaction hash.
+        :param provider: the name the child tree name.
+        :param tree: tree of inheritance


remove line above

rpytel1 · 2018-12-11T17:14:54Z

plebnet/clone/create-child.sh

@@ -29,7 +29,9 @@ export DEBIAN_FRONTEND=noninteractive
 cd

 CHILD_DNA_FILE=~/.config/Child_DNA.json
+#CHILD_QTABLE_FILE=


@HusainKapadia why is this commented?

fixed properly

Because when I wrote this the QTable.json had not been created. I'll add the correct paths if you haven't done so yet

rpytel1 · 2018-12-11T17:17:54Z

tests/agent/test_core.py

@@ -228,65 +227,65 @@ def test_update_offer(self):
        cloudomate_controller.update_offer = self.uo
        PlebNetConfig.save = self.save

-    def test_attempt_purchase(self):
+    def test_attempt_purchase(self):        


@HusainKapadia I think it is better not to comment code but to remove it.

tests/clone/test_server_installer.py

tests/controllers/test_cloudomate_controller.py

devos50 · 2018-12-14T08:48:48Z

It seems that the Plebnet job on Jenkins is filling up slots on our machines (because it is stuck): https://jenkins-ci.tribler.org. Please have a look at this 👍

MattSkala · 2018-12-14T11:18:40Z

It would be nice to mock cloudomate calls in tests so we have true unit tests. Right now the tests for QTable would break if any of cloudomate providers breaks, which probably should not be the case.
Check what happens when a provider in cloudomate breaks (e.g. get_options raises an exception, purchase fails). The bot should be resilient and handle such cases by decreasing a reward and trying a different provider.
Check if the calculate_measure is working correctly for unmetered bandwidth (e.g. linevast) and write a test case for that.
I don't like how dictionary is used everywhere so the code is not type-safe (very JavaScript-ish :D). It would be better to use custom types or named tuples for provider offers and state.
How are we going to handle VPN selection? Currently it's hardcoded to always use AzireVPN. It makes sense to use the cheapest VPN which is working for the selected VPS provider. It would be interesting if the bot can also learn which VPS providers do not care about traffic so we can save on VPN, and for which the VPN is a necessity for survival. Is it possible to integrate it somehow with the current QTable/should we create a separate QTable for VPN selection?

MattSkala · 2018-12-14T11:24:33Z

@devos50 @Jaapp- It looks like https://jenkins-ci.tribler.org/job/GH_Plebnet_PR_Tests/job/GH_PlebNet_coverage_check/17/console is stuck and all subsequent coverage check jobs are waiting for it to finish. Is it possible to kill the build now and set a timeout so it does not happen in future?

devos50 · 2018-12-14T13:43:35Z

@MattSkala yes. @Jaapp- has login credentials for Jenkins so he can kill these jobs. In the job settings, you can specify either a fixed timeout or a dynamic timeout (that is based on the duration of previous runs).

rpytel1 · 2018-12-16T12:13:02Z

@MattSkala I will try to do the 1-4 of your remarks now as I agree with them. Ad 5 I am not sure it is not better to do as seperate PR in the next sprint :)

It would be nice to mock cloudomate calls in tests so we have true unit tests. Right now the tests for QTable would break if any of cloudomate providers breaks, which probably should not be the case.

Check what happens when a provider in cloudomate breaks (e.g. get_options raises an exception, purchase fails). The bot should be resilient and handle such cases by decreasing a reward and trying a different provider.

Check if the calculate_measure is working correctly for unmetered bandwidth (e.g. linevast) and write a test case for that.

I don't like how dictionary is used everywhere so the code is not type-safe (very JavaScript-ish :D). It would be better to use custom types or named tuples for provider offers and state.

How are we going to handle VPN selection? Currently it's hardcoded to always use AzireVPN. It makes sense to use the cheapest VPN which is working for the selected VPS provider. It would be interesting if the bot can also learn which VPS providers do not care about traffic so we can save on VPN, and for which the VPN is a necessity for survival. Is it possible to integrate it somehow with the current QTable/should we create a separate QTable for VPN selection?

HusainKapadia · 2018-12-16T14:19:34Z

@MattSkala I will try to do the 1-4 of your remarks now as I agree with them. Ad 5 I am not sure it is not better to do as seperate PR in the next sprint :)

It would be nice to mock cloudomate calls in tests so we have true unit tests. Right now the tests for QTable would break if any of cloudomate providers breaks, which probably should not be the case.

Check what happens when a provider in cloudomate breaks (e.g. get_options raises an exception, purchase fails). The bot should be resilient and handle such cases by decreasing a reward and trying a different provider.

Check if the calculate_measure is working correctly for unmetered bandwidth (e.g. linevast) and write a test case for that.

I don't like how dictionary is used everywhere so the code is not type-safe (very JavaScript-ish :D). It would be better to use custom types or named tuples for provider offers and state.

How are we going to handle VPN selection? Currently it's hardcoded to always use AzireVPN. It makes sense to use the cheapest VPN which is working for the selected VPS provider. It would be interesting if the bot can also learn which VPS providers do not care about traffic so we can save on VPN, and for which the VPN is a necessity for survival. Is it possible to integrate it somehow with the current QTable/should we create a separate QTable for VPN selection?

@rpytel1 would you like to split the work? We can discuss about it tomorrow after or before the meeting.

rpytel1 · 2018-12-16T14:26:42Z

@HusainKapadia no, I prefer to do it alone and finish it today.

MattSkala · 2018-12-16T14:51:27Z

I created a separate issue for the VPN selection: #49

Also, please rebase to the current develop and try to investigate why tests are failing on the CI: https://jenkins-ci.tribler.org/job/GH_Plebnet_PR_Tests/job/GH_PlebNet_tests/21/console

rpytel1 · 2018-12-16T17:38:31Z

I think I did changes matching the remarks of @MattSkala. I did also rebase and resolved conflicts with develop.
There is one think I am currently unsure of: if we create child configuration, it comes with variable provider . Before in DNA there was only an info about the provider, without info about specific offer.
@HusainKapadia if you want you can check it now and make fix. I need to spend some time on other matters, but will probably check it during a night.

I don't know why two tests are crashing on Jenkins :(

MattSkala · 2018-12-17T11:22:16Z

Tests are failing because you are replacing cloudomate_controller module functions by a mock, so it influences other tests using those functions as well. Take a look at unittest.mock.patch ;)

rpytel1 · 2018-12-31T18:21:46Z

PR is working with test fixed still in 2018 :D
Feel free to review it last time before merge :)
If all will be great, let me know and then I will make all this 11 commits into one :)

MattSkala · 2019-01-06T19:40:27Z

Note that pickle does not serialize to JSON, but to a custom binary format. So the json_file variable name and the file name itself are misleading. For JSON serialization you would need to use the jsonpickle library instead. If you ask me, I'd prefer using JSON for easier debugging.
When running plebnet check, I get the following error:

File "/usr/local/lib/python2.7/dist-packages/plebnet/cmdline.py", line 41, in execute
    args.func()
  File "/usr/local/lib/python2.7/dist-packages/plebnet/cmdline.py", line 66, in execute_check
    agent.check()
  File "/usr/local/lib/python2.7/dist-packages/plebnet/agent/core.py", line 113, in check
    select_provider()
  File "/usr/local/lib/python2.7/dist-packages/plebnet/agent/core.py", line 318, in select_provider
    if providers >= 1 and sum(providers.values()) > 0:
TypeError: unsupported operand type(s) for +: 'int' and 'ABCMeta'

Please resolve the merge conflict by rebasing to the latest develop.

rpytel1 · 2019-01-07T23:10:46Z

I think it all works but would appreciate if someone (@MattSkala) would check it (plebnet check) :)

MattSkala · 2019-01-09T12:25:12Z

plebnet/agent/core.py

@@ -318,19 +320,17 @@ def select_provider():
    """
    if not config.get('chosen_provider'):
        logger.log("No provider chosen yet", log_name)
-        all_providers = dna.vps
+        all_providers = cloudomate_controller.get_vps_providers().keys()
        excluded_providers = config.get('excluded_providers')
        available_providers = list(set(all_providers.keys()) - set(excluded_providers))
        providers = {k: all_providers[k] for k in all_providers if k in available_providers}

        if providers >= 1 and sum(providers.values()) > 0:


I think the condition should be len(providers) >= 1

MattSkala · 2019-01-09T13:18:30Z

Some notes after a discussion and deeper inspection of the code:

In update_values you should consider the option currently being purchased. If the purchase succeeds, the reward for the particular provider option should be increased, otherwise decreased.
In install_available_servers, a string is passed as a provider parameter to qtable.create_child_qtable, but I think VPSState object is expected. For this, you need to store the option name in cloudomate_controller.purchase_choice to the list of bought services.
In both qtable and environment dictionaries, you are using provider_offer.name as the key. What if more providers are using the same name for their vps options? I think the key should be the combination of both the provider name and option name.
The bandwidth can be unmetered (e.g. see linevast). Currently, it is set to MIN_BANDWIDTH in ProviderOffer.__init_. Instead, I'd replace unmetered bandwidth with an UNLIMITED_BANDWIDTH constant with a reasonable value (10 TB?), so the measure is calculated properly for such offers.
In cloudomate_controller.py in the options function, I would put provider.get_options() call in a try block to protect against cloudomate failures.

rpytel1 · 2019-01-09T20:51:33Z

Ad 1:
current_state - current offer the program is in
next_state - offer program is trying to purchase
As I recall we were discussing that about one month ago. We decided that if we can't manage to buy chosen offer(next_state) it means that current state(next_state) is not enabling us to replicate. That's why all the entries are updated transitioning to current_state are negatively updated.
What we can do is to mantain what we have and extend it with the significant negative update of the transition: current_state -> next_state

rpytel1 · 2019-01-09T21:51:16Z

Ad 5: Is it really Pull Request related?

rpytel1 · 2019-01-09T22:19:30Z

2-4 fixed and tested. I will wait until we decide what to do with 1 and 5 :)

MattSkala · 2019-01-10T22:44:25Z

Ad 1:

We decided that if we can't manage to buy chosen offer(next_state) it means that current state(next_state) is not enabling us to replicate.

Ok, I think I understand the original motivation now. It probably makes sense for positive updates (we were able to buy the current option, install PlebNet on it, and earn enough BTC).

The negatives update is more complicated though. There are 2 types of failures:

We were not able to earn enough BTC with the current state. In such a case, the condition checking the balance in attempt_purchase would not be evaluated to true in the first place, so no updates occurs anyway. We could probably handle this somehow, but until we have a bot gossip implemented, it does not make much sense to consider this as the bot will die anyway and the table won't be propagated to others.
We were able to earn enough BTC, but the purchase_choice method fails. This means that either the selected provider is broken in cloudomate, or the provider blocks purchases from our IP address. So I think it makes sense to negatively update only the transition from the current state to the chosen offer.

Currently if we fail to purchase a chosen offer, we penalize even the current state for that, which I think is probably not right.

Ad 5: Not really, we can also create a separate issue for that, but I think it's just a two-line change.

MattSkala · 2019-01-10T22:46:13Z

Also, I had a look at the tests and I think the asserted conditions could be somewhat stronger. Especially in test_update_environment and test_update_values, the test only checks if the values changed. A good test should also check if the values changed correctly.

MattSkala · 2019-01-10T23:01:39Z

Is 2) really fixed? I can't find any related changes in the last commit. Maybe you forgot to push it.

Ad 4) I think the bandwidth is in TB, so the constant value should be 10 instead of 10000.

synctext · 2019-01-11T05:04:30Z

595 additional lines, becoming impressive! Code coverage tracking would be great at some point.

rpytel1 · 2019-01-11T08:50:30Z

AD1: In my opinion it makes sense what you wrote, so I fixed it accordingly.
AD2: You were right, I thought I added it. Changes related are added in the latest commit.
AD4: fixed, my mistake
AD5: ok, fixed
I extended the functionality of tests not to just check if they are not the same but also if they are greater or equal from the before and as well certain value checks are introduced.

Let me know when PR is ready to be merged so I will make everything in one commit with meaningful code message

MattSkala · 2019-01-13T12:06:55Z

@rpytel1 Looks good, I think it is ready to merge. We can do any further changes (simulated annealing) as a separate PR. Can you please squash the commits into one?

…o QTable.

Tested nodes messaging

rpytel1 force-pushed the rudimentary_qTable branch from d74284c to 1b6f50d Compare December 4, 2018 17:22

synctext reviewed Dec 10, 2018

View reviewed changes

synctext mentioned this pull request Dec 10, 2018

Bitcoin-Intelligent-Life - Blockchain engineering class project Tribler/tribler#4039

Closed

4 tasks

rpytel1 self-assigned this Dec 11, 2018

rpytel1 reviewed Dec 11, 2018

View reviewed changes

tests/clone/test_server_installer.py Outdated Show resolved Hide resolved

rpytel1 reviewed Dec 11, 2018

View reviewed changes

tests/clone/test_server_installer.py Outdated Show resolved Hide resolved

rpytel1 reviewed Dec 11, 2018

View reviewed changes

tests/clone/test_server_installer.py Outdated Show resolved Hide resolved

rpytel1 reviewed Dec 11, 2018

View reviewed changes

tests/controllers/test_cloudomate_controller.py Outdated Show resolved Hide resolved

rpytel1 force-pushed the rudimentary_qTable branch from 6b7b74b to 645418d Compare December 13, 2018 16:17

rpytel1 force-pushed the rudimentary_qTable branch from adfbb8c to c45870d Compare December 16, 2018 17:28

MattSkala force-pushed the rudimentary_qTable branch from a1de0b2 to c45870d Compare December 19, 2018 14:22

rpytel1 force-pushed the rudimentary_qTable branch 2 times, most recently from c0b9f4d to fb89e76 Compare December 31, 2018 18:16

rpytel1 force-pushed the rudimentary_qTable branch 3 times, most recently from 1d24162 to b550420 Compare January 7, 2019 16:15

rpytel1 force-pushed the rudimentary_qTable branch from 857bbde to 234b7a6 Compare January 9, 2019 11:18

MattSkala commented Jan 9, 2019

View reviewed changes

rpytel1 force-pushed the rudimentary_qTable branch from 2258964 to d716896 Compare January 11, 2019 08:48

rpytel1 force-pushed the rudimentary_qTable branch 2 times, most recently from d8356fa to f3f04d3 Compare January 11, 2019 09:05

Introducing QTable, global changes to accomodate migration from DNA t…

7615d8b

…o QTable.

rpytel1 force-pushed the rudimentary_qTable branch from f3f04d3 to 7615d8b Compare January 13, 2019 12:29

MattSkala changed the title ~~WIP: Rudimentary q table~~ Rudimentary q table Jan 13, 2019

MattSkala merged commit ebce820 into develop Jan 13, 2019

MattSkala deleted the rudimentary_qTable branch January 21, 2019 10:49

devos50 pushed a commit that referenced this pull request Jul 16, 2020

Merge pull request #42 from GioAc96/nodes-messaging

bd2404f

Tested nodes messaging

Rudimentary q table #42

Rudimentary q table #42

Conversation

MattSkala commented Dec 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpytel1 commented Dec 11, 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

devos50 commented Dec 14, 2018 • edited Loading

MattSkala commented Dec 14, 2018 • edited Loading

MattSkala commented Dec 14, 2018

devos50 commented Dec 14, 2018

rpytel1 commented Dec 16, 2018

HusainKapadia commented Dec 16, 2018

rpytel1 commented Dec 16, 2018

MattSkala commented Dec 16, 2018

rpytel1 commented Dec 16, 2018 • edited Loading

MattSkala commented Dec 17, 2018

rpytel1 commented Dec 31, 2018

MattSkala commented Jan 6, 2019

rpytel1 commented Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

MattSkala commented Jan 9, 2019 • edited Loading

rpytel1 commented Jan 9, 2019

rpytel1 commented Jan 9, 2019

rpytel1 commented Jan 9, 2019

MattSkala commented Jan 10, 2019

MattSkala commented Jan 10, 2019

MattSkala commented Jan 10, 2019

synctext commented Jan 11, 2019

rpytel1 commented Jan 11, 2019 • edited Loading

MattSkala commented Jan 13, 2019

MattSkala commented Dec 3, 2018 •

edited

Loading

devos50 commented Dec 14, 2018 •

edited

Loading

MattSkala commented Dec 14, 2018 •

edited

Loading

rpytel1 commented Dec 16, 2018 •

edited

Loading

rpytel1 commented Jan 7, 2019 •

edited

Loading

MattSkala commented Jan 9, 2019 •

edited

Loading

rpytel1 commented Jan 11, 2019 •

edited

Loading