Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored code and added some exceptions avogadro-remote.py #406

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

adityaomar3
Copy link
Contributor

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 adityaomar3 changed the title Refactored/added some exceptions avogadro-remote.py Refactored code and added some exceptions avogadro-remote.py Sep 19, 2023
@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Sep 21, 2023

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:

scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
@adityaomar3
Copy link
Contributor Author

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

@ghutchis
Copy link
Member

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
Copy link
Contributor Author

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

@adityaomar3 adityaomar3 force-pushed the master branch 2 times, most recently from a0a6cc1 to c01367e Compare September 30, 2023 18:58
@adityaomar3
Copy link
Contributor Author

@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
Copy link
Member

ghutchis commented Oct 1, 2023

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
Copy link
Contributor Author

adityaomar3 commented Oct 2, 2023

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
Copy link
Member

ghutchis commented Oct 2, 2023

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

@adityaomar3
Copy link
Contributor Author

adityaomar3 commented Oct 2, 2023

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
Copy link
Member

ghutchis commented Oct 2, 2023

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
Copy link
Member

ghutchis commented Oct 2, 2023

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

@ghutchis
Copy link
Member

ghutchis commented Oct 2, 2023

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

@adityaomar3
Copy link
Contributor Author

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
Copy link
Member

ghutchis commented Oct 2, 2023

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

@adityaomar3
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@ghutchis
Copy link
Member

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

scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
self.sock.connect(tempfile.gettempdir() + "/" + name)
# print the connection statement
print("CONNECTION ESTABLISHED SUCCESSFULLY")
except Exception as exception:

Check notice

Code scanning / Pylintpython3 (reported by Codacy)

Catching too general exception Exception Note

Catching too general exception Exception
scripts/avogadro-remote.py Fixed Show fixed Hide fixed
adityaomar3 and others added 6 commits October 24, 2023 15:33
Refactored the complete code into connection class and added the check for socket connection

Signed-off-by: Omar <[email protected]>
Signed-off-by: aditya <[email protected]>
Signed-off-by: Omar <[email protected]>
Signed-off-by: Omar <[email protected]>
Signed-off-by: aditya <[email protected]>
Signed-off-by: Omar <[email protected]>
methods to one private method

Signed-off-by: Omar <[email protected]>
Signed-off-by: aditya <[email protected]>
Signed-off-by: Omar <[email protected]>
… completely functional

Signed-off-by: aditya <[email protected]>
Signed-off-by: Omar <[email protected]>
Signed-off-by: aditya <[email protected]>
Signed-off-by: Omar <[email protected]>
@adityaomar3
Copy link
Contributor Author

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.

@ghutchis ghutchis merged commit ea4e77c into OpenChemistry:master Oct 25, 2023
28 of 29 checks passed
@welcome
Copy link

welcome bot commented Oct 25, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants