Refactored code and added some exceptions avogadro-remote.py

[adityaomar3]

Refactored the complete code into connection class and added the check for socket connection

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

[adityaomar3]

Functionalities/changes required:
[ ]1. It would be great to refactor the entire connection class so that the methods take arguments and use Pythonic method names, like:
[x]open_file(filename)
[x]save_graphic(filename)
[ ]Add methods for all the current RPC methods:
[x]kill
[x]openFile(filename)
[ ]loadMolecule(string)
[x]saveGraphic(filename)
[ ]exportFile(filename)

[x]2. Error detection in init if the socket cannot be created.

[ ]3. Merge the current send_message and receive_message methods (which handle the low-level interaction with the connection) into the _json methods.

[ ]4. Create new "high level" send_message(method, params = {}) method which formats the JSON template to minimize code duplication:

[adityaomar3]

Also if you can help me with the functions loadMolecule and exportFile will do, so that I can implement them @ghutchis

[ghutchis]

Also if you can help me with the functions loadMolecule and exportFile will do, so that I can implement them @ghutchis

I thought I was pretty clear in my other commends:

• loadMolecule() will send the file as a string as part of the content in a json dictionary, with a format parameter in the json as well.
• exportFile() expects a fileName parameter sent with the JSON.

I don't care as much about all the methods as far as points 3, 4 and the general code scanning comments from Codacy. (Let me know if you don't see them on the pull request)

[adityaomar3]

There are few codacy comments but they are outdated, I think I am not able to see the latest ones

[adityaomar3]

@ghutchis I have done the final refactoring from my side, latest codacy comments are not visible
Also there is a method recv_json which was not used throughout the old code, I have added it in the refactored one but I have commented it, what was it for previously?

[ghutchis]

To be fair, I didn't write the original code. But recv_json should be receive_json and builds on receive_message (the low-level "get data from the socket") method.

It's not used because those particular methods are one-way .. send something to the app and not receive anything in return.

Some time-intensive things (e.g., "render a molecular surface .. tell me when you're done") need both a send and a receive component:

# this doesn't exist at the moment, it's just example pseudo-code
render_surface()
response = receive_message()
# check response json for error / etc

I do think the size for the receive_message is really small (1024 bytes). It should probably be something like 16384 or 32768 to be sure larger messages can be sent.

[adityaomar3]

was this script used to run previously?
Because its denying connection,
Also if you can help me with which server or where the socket connection has to be made
Currently it is set to some temporary created directry tempfile.gettempdir() + "/" + name

[ghutchis]

The script works. You have to run the Avogadro app - it will create the socket when running.

[adityaomar3]

PS C:\project\avogadroapp> python .\scripts\avogadro-remote.py Traceback (most recent call last): conn = Connection() File ".\scripts\avogadro-remote.py", line 21, in __init__ self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM) AttributeError: module 'socket' has no attribute 'AF_UNIX'

When I corrected UNIX it throws

PS C:\project\avogadroapp> python .\scripts\avogadro-remote.py error while connecting: connect(): AF_INET address must be tuple, not str
Then I added port 0 to make it a tuple but it throws error.
PS C:\project\avogadroapp> python .\scripts\avogadro-remote.py error while connecting: [Errno 11001] getaddrinfo failed

Please help me with this

Also which OS are you using?

[ghutchis]

I use Mac and Linux. A local socket is also known as a named pipe - it's a temporary file buffer used for inter-process communication. You don't want a port.

https://wiki.wireshark.org/CaptureSetup/Pipes#way-3-python-on-windows

Looking around I think you can try something like this on Windows:
f = open(r'\\.\pipe\avogadro', 'r+b', 0)

As per:
http://jonathonreinhart.blogspot.com/2012/12/named-pipes-between-c-and-python.html

[ghutchis]

My understanding was that the interprocess communication was tested on Windows, but clearly we need to add some tests on all platforms!

[ghutchis]

Yes, sadly it seems as if Python doesn't support that on Windows: python/cpython#77589

[adityaomar3]

Its totally fine, I think its time to switch to ubuntu.
Ubuntu 20.04 is fine according to you ?
I Will test all of this on windows and try to configure it for windows too for sure but bit later, firstly I think things should start working out.

[ghutchis]

I would definitely like to know about the open(r'\\.\pipe\avogadro', 'r+b', 0) option, since that seems useful.

[adityaomar3]

I would definitely like to know about the open(r'\\.\pipe\avogadro', 'r+b', 0) option, since that seems useful.

Sure! First I will try this.

[adityaomar3]

File ".\scripts\script-test.py", line 26, in __init__ f = open(r'./pipe/avogadro', 'r+b', 0) FileNotFoundError: [Errno 2] No such file or directory: './pipe/avogadro'

I think it needs the exact path but as I could see in the previous code this was the temp dir we were creating?

[adityaomar3]

@ghutchis sir is this script ready to merge ?
can you please review it

[ghutchis]

Thanks, I will take a look tomorrow (24 Oct)

[adityaomar3]

Thanks, I will take a look tomorrow (24 Oct)

Sure sir, I have observed you are quiet busy with university and this app, also I didn't saw others much active here,
review it for the merge whenever you get time.
until then will search for other issues and try to solve them. Or any important issue you need to assign, can work on that too.

[welcome]

Congrats on merging your first pull request! 🎉 Thanks for making Avogadro better for everyone!