tcpexmachina / remy

403 stars 78 forks source link

Config vector to Replace config range #40

Closed deeptir18 closed 8 years ago

deeptir18 commented 8 years ago

-Updated the 'configrange' object to have a vector of netconfigs instead of ranges over the different network parameters -Updated protobuf format for configrange -Updated remy and remy-poisson to be compatible with new protobuf format -Scoring files (sender runner and scoring example) still take in command line arguments but internally defines configrange for evaluator differently, to be compatible

-Update inputconfigrange executable to take in the same parameters, but write a protobuf with the new structure

czlee commented 8 years ago

It's not obvious to me why this failed. The symptom is that, when the Python script tries to parse sender-runner's output, the regular expression that tries to match the "link packets per ms" specification (below), it matches multiple times. The relevant part of the log is: https://travis-ci.org/tcpexmachina/remy/builds/114592384#L1116-L1125 (The second traceback is the consequence of the first traceback, so the root cause is the first traceback.)

Here's the regular expression in question:

LINK_PPT_PRIOR_REGEX = re.compile("^link_packets_per_ms\s+\{\n\s+low: (-?\d+(?:\.\d+)?)\n\s+high: (-?\d+(?:\.\d+)?)$", re.MULTILINE)

To give some context: the plotting script highlights on the plot the training (link speed) range of the RemyCC that was given to it. So it scrapes the sender-runner output for this, then uses it to shade that region in light gray. This concept (highlighting the range) might not make sense if config range is deprecated in favor of config vectors. We'll then need to find some alternate way of representing the config vector on the plot. Any thoughts?

keithw commented 8 years ago
  1. Probably best simply to highlight the min and max values in the vector (pretending like it's still a range).
  2. Since the Python code already speaks protobuf, would be preferable if it got this by reading the protobuf itself instead of scraping the sender-runner output with regexes.
  3. Any idea why the test took 42 minutes to fail? That's perhaps more worrisome to me than the fact that it did. :-) We do want to keep the total test time relatively low (ideally <10 minutes) so that working on this codebase does not become an unhappy adventure for months and years to come.
keithw commented 8 years ago

.4. Deepti, if you tune in to the Slack channel (https://keithw.slack.com/messages/edgect/) you will get an alert when these tests pass or fail and can discuss with others.

czlee commented 8 years ago

Re (2)—this Python code actually doesn't speak protobuf (yet), I only started getting Python to read protobufs when I started on the logging work, which is on my own separate branch (I started refactoring today). I agree in principle, but the protobuf infrastructure isn't really in place on the main branch yet.

(Also, when I wrote that plotting script, I wrote it in Python 3, before I knew that protobufs only has good support for Python 2. But that should be easy to change.)

deeptir18 commented 8 years ago

Is there anything I should do in the pull request to make it compatible with the test?

keithw commented 8 years ago

For a PR that changes the interface to the existing tools (like this one), you'll need to change the calling code so that it still works, or at least does something similar. You can run make -j check to run the tests locally (or make distcheck for the most careful tests).

My recommendation: just have sender-runner print out the same line that it prints out before (the one that the test is looking for). Find the smallest and biggest value in the vector and pretend that's a range and print it out as before. That way everybody is happy and it seems like a minimum of work.