Headless exec #6

degv364 · 2018-02-24T06:17:56Z

@arcoslab/simulator-coordinators This fixes #4

This also includes the futurization done by @liewhnic

I am adding a new script that opens the same ports as pyrovito, but instead of creeating the visualization, it logs waht it gets from the ports.

Futurize stage1 changes applied to all python files. pyrovito_utils.py left the same by futurize.

Extra "#!/usr/bin/env" line was added by futurize but makes __init__.py break flake8 statements. So removed.

Extra parenthesis were added at futurize process. Removed since unnecesary.

"# -*- coding: utf-8 -*-" line is added in order to indicate the respective file encoding.

A scripts that creates all the yarp interfaces that normal pyrovito would create, but instead of providing a visualization, it logs all the info gathered from its opened ports. This is the first step towards Headless execution.

jprltsnz

Also PEP-8 makes no recommendation on string quotes (relevant part of PEP-8, also note the exception to the rule), but I REALLY dislike the lack of consistency. Please turn all your strings to either use single quote '' or dobule quotes, whatever you choose I'll add to our python code styling guide.

Please rebase this branch on master to keep the commits order organized now that stage1 branch was meged

jprltsnz · 2018-02-24T18:17:21Z

setup.py

@@ -11,4 +12,4 @@
      url='http://www.arcoslab.org/',
      package_dir={'pyrovito': ''},
      packages=['pyrovito'],


This will only package pyrovito, not your new script. I suggest changing to the src/ directory structure:

├── COPYING ├── Makefile ├── README.md ├── setup.py └── src └── pyrovito ├── __init__.py ├── pyrovito_utils.py └── scripts ├── __init__.py ├── pyrovito └── vis_sink.py

And adjust the setup.py accordingly

jprltsnz · 2018-02-24T18:18:34Z

setup.py

@@ -11,4 +12,4 @@
      url='http://www.arcoslab.org/',
      package_dir={'pyrovito': ''},
      packages=['pyrovito'],
-      scripts=['pyrovito'])
+      scripts=['pyrovito', 'vis_sink.py'])


Since this is new code I'd rather you used a console_scripts entry points (link to docs) instead of the old scripts way. If you follow the file struture suggestion it could be something like this:

entry_points={ 'console_scripts': [ 'vis_sink'='pyrovito.scripts.vis_sink:main' ' pyrovito'='pyrovito.scripts.pyrovito:main' ] },

And move the top level instructions (the ones with no indent) to a main function

I don't understand what is the point with this. What to we gain? just removing the .py at the end?

I would like to clarify you could remove the .py form the script name yourself if you wanted and it is a minor detail, it is nice since *NIX shell executables by convension don't use it, but it is a minor detail.

console_scripts require minimal changes to be supported, the directory structure that I suggest is just that, a suggestion. All you need to do is put the top level module instructions on a function.

console_scripts entry point manages a few things that scripts doesn't regarding operating system interoperability. You could argue that this is not important for us since we only use Debian, and you'd be right, however this is also a free software project and it might be useful for someone else and it takes little effort to support doing this the right way. Similarly, I think doing things that brings extra unsued advantages that bring no disadvantages with them is useful since we might use them or need them in the future, specially for low effort things like this one.

console_scripts IS right way™️ to do this. console_scripts is the recommended way to provide executable scripts and in line with best practices regarding packaging in the python community, which we, ARCOS-Lab, internally agreed to follow, this alone should be enough argument.

script is actually a legacy feature. It won't probably be removed in the future so it's save to keep using, but given the chance, and in general, prefer coding using with non-legacy/recommended features unless there is a justification not to. Given the low effort for the change I don't think that using the legacy scripts feature is justified.

Hope this clears my positions regarding this.

jprltsnz · 2018-02-24T18:19:59Z

vis_sink.py

+
+from arcospyu.control.control_loop import Controlloop
+
+sys.path.append('../hand_cart')


This depends on the file being ran form a specific directory or else this will fail...

I don't see a good instance where this is a good idea. I know a lot of our repos has things like this one but lets not copy bad methods.

Maybe passing the path to this as a commandline option?

jprltsnz · 2018-02-24T18:21:45Z

vis_sink.py

+from arcospyu.control.control_loop import Controlloop
+
+sys.path.append('../hand_cart')
+sys.path.append('../config_data/lwr')


Same as above

jprltsnz · 2018-02-24T18:24:06Z

vis_sink.py

+    parser = optparse.OptionParser(
+        'usage: %prog [options]', add_help_option=False)
+    parser.add_option('-e', '--help', action='help')
+    parser.add_option(


All of this help strings are bad. Most of them are things I could infer from the option names...

jprltsnz · 2018-02-24T18:39:19Z

vis_sink.py

+                thumb_speed = 0.0  # TODO: Calculate thumb speed
+                self.add_log({
+                    "id":
+                    "Hand" + side_str,


Use ''.join() as mentioned above.

jprltsnz · 2018-02-24T18:41:02Z

vis_sink.py

+                    "Hand" + side_str,
+                    "finger":
+                    finger,
+                    "angles": ([thumb_angle] + list(


lists have an extend() method that is better suited for this task: docs

This would require to define a new variable, just for using extend. I do not see any performance issues or formatting issues associating to adding lists. I prefer to leave it as it is...

Actually you are right I hadn't consider that, I'll remove the comments since I think it's preferable to use + than to create single use variables.

In the general sense (when there are no single use variables) do think is preferable to use extend

jprltsnz · 2018-02-24T18:41:29Z

vis_sink.py

+            else:
+                self.add_log({
+                    "id":
+                    "Hand" + side_str,


Use ''.join() as mentioned above.

jprltsnz · 2018-02-24T18:43:11Z

vis_sink.py

+
+    def terminate_handler(self, signum, stack_frame):
+        print(('Catched signal', signum))
+        log_file = open("log_file_" + str(time.time()), 'w')


Use ''.join() as mentioned above.

jprltsnz · 2018-02-24T18:43:59Z

vis_sink.py

+        sys.exit()
+
+
+loop = VisSinkLoop(15.)


put this inside a main() function to use the console_script entry point

jprltsnz · 2018-02-27T03:52:36Z

vis_sink.py

        default=False,
-        help='arm right side')
+        help="Enable the configuration and simulation or a right arm")


Typo here should be: simulation of a hand. Just commenting since it is easy to miss.

hgeihsen and others added 9 commits February 20, 2018 17:44

dev: Futurize stage1 applied

7f27075

Futurize stage1 changes applied to all python files. pyrovito_utils.py left the same by futurize.

fix: flake8 compliance

b229609

Extra "#!/usr/bin/env" line was added by futurize but makes __init__.py break flake8 statements. So removed.

fix: extra parenthesis removed

f6281d7

Extra parenthesis were added at futurize process. Removed since unnecesary.

dev: Encoding indication is added to python files

02a1b05

"# -*- coding: utf-8 -*-" line is added in order to indicate the respective file encoding.

Add vis sink

4ec43aa

A scripts that creates all the yarp interfaces that normal pyrovito would create, but instead of providing a visualization, it logs all the info gathered from its opened ports. This is the first step towards Headless execution.

Add vis_sink to README

5c17fe6

Merge futuratization into headless-exec

a52863d

Fix readme format

1605c9e

Futurize vis_sink

89d664b

degv364 added the enhancement label Feb 24, 2018

degv364 requested a review from a team February 24, 2018 06:17

Add encoding comment to vis_sink

45bb94d

jprltsnz suggested changes Feb 24, 2018

View reviewed changes

degv364 added 3 commits February 26, 2018 19:00

Use double quotes

56ef0bd

substitute all string sums with joins

2a8d7a6

Add a main function

62526f2

jprltsnz reviewed Feb 27, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Headless exec #6

Headless exec #6

degv364 commented Feb 24, 2018

jprltsnz left a comment

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

degv364 Feb 27, 2018

jprltsnz Feb 27, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

degv364 Feb 27, 2018

jprltsnz Feb 27, 2018 •

edited

Loading

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 24, 2018

jprltsnz Feb 27, 2018 •

edited

Loading


		from arcospyu.control.control_loop import Controlloop

		sys.path.append('../hand_cart')

Headless exec #6

Are you sure you want to change the base?

Headless exec #6

Conversation

degv364 commented Feb 24, 2018

jprltsnz left a 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

jprltsnz Feb 27, 2018 • 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

jprltsnz Feb 27, 2018 • edited Loading

Choose a reason for hiding this comment

jprltsnz Feb 27, 2018 •

edited

Loading

jprltsnz Feb 27, 2018 •

edited

Loading