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

Py3 dbs3 client - setup changes #649

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

todor-ivanov
Copy link

@todor-ivanov todor-ivanov commented Apr 28, 2021

READY!

Those are changes needed to fix only the rpm build process for the py3-dbs3-client.spec and py3-dbs3-pycurl-client.spec from the cmsdist repository

@amaltaro
Copy link
Contributor

Nice catch Todor.

@yuyiguo Yuyi, do you think we could skip building the DBS documentation for a while? Maybe we could create another GH issue to track it in the near future? Please let us know what your thoughts are.

@yuyiguo
Copy link
Member

yuyiguo commented Apr 29, 2021

@todor-ivanov @amaltaro
Please go head w/o building DBS doc. I will create GH issue to reminder us for putting back the automatically generated docs back.

@yuyiguo
Copy link
Member

yuyiguo commented Apr 29, 2021

FYI
#650

@todor-ivanov
Copy link
Author

Thanks @yuyiguo!
BTW There are going to be some more changes to be added to the current PR. I will write here again shortly.

@todor-ivanov
Copy link
Author

@vkuznet @yuyiguo @amaltaro please take a look at the jsonwrapper suggested here - Still alot of testing to be done

@vkuznet
Copy link
Contributor

vkuznet commented Apr 30, 2021

@todor-ivanov , few things:

  • the standard json library only provide load, loads, dump and dumps, the encode and decode methods only belongs to cjson. Therefore, if you indent to replace cjson.encode and cjson.decode in DBS code it is better to replace them with jsonwrapper.loads and jsonwrapper.dumps methods to stay consistent with APIs of standard json library
  • there is no need to have other json's implementation if you never use them. The jsonwrapper provides examples how to add another json implementation, and if necessary it can be done. Therefore you may simplify jsonwrapper to eliminate other json's implementation
  • the PR provides not related to json transition changes and it is better to leave them aside, but I know that DBS codebase is not ideal. For instance, there is a mix of spaces and tabs in a code which makes it very hard to read and deal with in vim. But it is another story.

@todor-ivanov
Copy link
Author

todor-ivanov commented Apr 30, 2021

Thnks @vkuznet !

the PR provides not related to json transition changes and it is better to leave them aside

The initial intention was to fix everything related to py3 migration in dbs3-client, but not only the cjson dependency

For instance, there is a mix of spaces and tabs in a code which makes it very hard to read and deal with in vim.

Not just hard to read. It actually prevents the code from running at all in ipython3, so those changes are indeed needed for the rest of the testing

When it comes to supporting not needed libraries I agree with you - I am going to simplify the wrapper, but I am about to leave ujson in because I found it a quite good substitute both interface related and performance related.

@yuyiguo
Copy link
Member

yuyiguo commented Apr 30, 2021

For instance, there is a mix of spaces and tabs in a code which makes it very hard to read and deal with in vim.

In the good old days, everyone used vi and defined their own indentations, some used spaces and some used tabs. Various people touched DBS code so these two are mixed in the code. What I did was that I use modern tools , such as MS VSC to format the entire file when I work on it.

@yuyiguo
Copy link
Member

yuyiguo commented Apr 30, 2021

I agreed with Valentin's suggestion on using standard json library with loads/dumps instead of cjson's encode/decode.

@vkuznet
Copy link
Contributor

vkuznet commented May 6, 2021

@todor-ivanov I just noticed that you put code into DBS/Client area while cjson is used both in DBS Server and Client. If you'll keep it in Client then DBS Server code will depend on client unless you'll duplicate the code. Instead, you should make an independent package and put it on pypi. Doing this way it will allow code (DBS Client and Server as well as WMCore) depend on it. The wrapper can be and should be (in my opinion) packaged independently from DBS codebase. But I'll leave a decision about it to WMCore group and @yuyiguo

@todor-ivanov
Copy link
Author

Hi @vkuznet @yuyiguo Thanks for the feedback. With my 5th commit to the current PR [1] I tried to simplify the jsonwrapper coul you please take a look and tell me if it is satisfactory enough, so I may continue with the rest of the code modifications for migrating the dbsclient to py3. As of the following:

  • the standard json library only provide load, loads, dump and dumps, the encode and decode methods only belongs to cjson. Therefore, if you indent to replace cjson.encode and cjson.decode in DBS code it is better to replace them with jsonwrapper.loads and jsonwrapper.dumps methods to stay consistent with APIs of standard json library

I agree it would be more close to the standard if we do so, but actually in the code there are several places [2] where it actually relies on the cjson.{Encode,Decode}Error exceptions. And because of that following Valentin's initial approach seemed reasonable to me - i.e. wrapping the standard .loads && .dumps calls with the .encode && .decode methods from the jsonwrapper, so that the exceptions coming from the actual json.loads && json.dumps calls can be enveloped (from a single place) with the common set of exceptions defined in the jsonwrrapper and then handled in the code the same way they were before. This way we avoid the need to juggle with exceptions in the dbsclient code. Otherwise in the DBS clinet code we will have to handle two types of exceptions - those from cjson and those from json.

[1]
f859582

[2]

except cjson.DecodeError:

@todor-ivanov
Copy link
Author

todor-ivanov commented May 6, 2021

Hi @vkuznet

I just noticed that you put code into DBS/Client area while cjson is used both in DBS Server and Client ...

I agree that the higher in the tree we put the jsonwrapper the better. My initial intention was not to solve all DBS python3 issues, and that's why I restrained myself only to the code base related to DBSClient, with the intend at some near future, the people responsible for DBS code to take care of positioning it at the right place at the right moment. @amaltaro @yuyiguo please let me know what you think.

@vkuznet
Copy link
Contributor

vkuznet commented May 6, 2021

Regarding exceptions. There are two pieces required. One the jsonwrapper should wrap them such that exception will come
from a single method, like load/loads or dump/dumps. Basically you need to catch cjson exception within jsonwrapper and then through again like json module does.

The second part is to modify DBS code itself to catch just common exception which will come either from json module or from jsonwrapper. This will solve issue with different json implementations.

@yuyiguo
Copy link
Member

yuyiguo commented May 6, 2021

@todor-ivanov

Restraining your current work on DBS client is what I think you may do now. I am thinking we should separate DBS client into a different package/repo from DBS server, but that is later. So we will reviste DBS client later, for now get it to work with py3 first. Then, WM can move forward quickly. Regarding the decode/encode, it is idea to follow py3 json as discussed, but we can live with the cjson style for now.

@amaltaro
Copy link
Contributor

amaltaro commented May 7, 2021

Todor and I had a chat yesterday and we seem to be aligned with what Yuyi said above. We should focus on the minimum amount of changes in dbs3-client and dbs3-pycurl-client to have it compatible with python3, especially because the rest of the DBS code future is also uncertain at this stage.

A few options for those minimum changes that come to my mind are:

  • provide this jsonwrapper to the dbs3-client/dbs3-pycurl-client packages only, leaving all the rest of DBS code untouched;
  • simply replace cjson by json in the dbs3-client/dbs3-pycurl-client code; and keep it in a separate branch.
  • if more change are needed to these client packages, we could consider as well a 2to3 conversion of the client code from python2 to python3; and keep it in a separate branch for now.

Just my 2cents.

@klannon
Copy link

klannon commented May 7, 2021

I fully agree. To reinforce what you said, for this purpose, there's no need to try to keep compatibility with Py2 (as we did for WMCore). We just need something (on a separate branch, if necessary) that works ASAP so that we can move forward with validation of WMCore under Py3.

@yuyiguo
Copy link
Member

yuyiguo commented May 7, 2021

I did both "future" and "2to3" on entire DBS code. The changes were not much in terms of types of changes. I think it is safe to move DBS client to py3 and put it in a separate branch.

@todor-ivanov
Copy link
Author

As discussed in a meeting we had between me, @amaltaro and @mapellidario this PR was split in two separate PRs:

  • The current PR provides the minimal changes in setup.py.
  • The new PR provides all the python3 related changes.

p.s. This PR is created against master and can safely be merged there - this will not break the py2 version of dbs-client

@todor-ivanov todor-ivanov changed the title Py3 dbs3 client Py3 dbs3 client - setup changes May 13, 2021
yuyiguo added a commit to yuyiguo/DBS that referenced this pull request May 17, 2021
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.

5 participants