-
Notifications
You must be signed in to change notification settings - Fork 108
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
Remove/replace MySQL-python library by something else that supports python3 #9805
Comments
From a quick look in WMCore, it seems this is the only place still depending on this library: WMCore/src/python/WMQuality/TestInit.py Line 23 in 2c67252
(which is currently gone) so it can be easily deprecated in WMCore. |
Currently, It does not seem that this dependency is playing a big role. It seems to be used just to set some metadata in the form of some class members, or to select some errorcode depending on the DB backend. We can either make some changes to state clearly that we are not using |
Hmm, from the package dependencies: it looks like such change in the REST module would affect:
I thought DBS was also relying on it... So my concern is with CRABServer (and maybe CRABCache). In case you come up with a PR to remove this functionality from WMCore, we should ping Stefano to test things before we merge anything. |
That is why I would actually only drop the dependency for now and leave all the code cleaning for the future. I assume that "discarding the dependency" for us only means
Am I correct? Do you see any problem in just changing our dependencies and trying to run our agents without that dependency before contacting others? |
This is related to dmwm#9805 We want not to depend anymore on MySQL-python, since it does not support python3. This is the only occurrence of this package left in dmwm/WMCore
This is related to dmwm#9805 We want not to depend anymore on MySQL-python, since it does not support python3. This is the only occurrence of this package left in dmwm/WMCore
I was going to remove |
@mapellidario I guess this issue is still opened because there was this pending item to remove the dependency from the spec files in cmsdist, right? |
Yes, you are correct. |
Sigh... I forgot testing it this week when a new WMAgent production release was made. Will try again in the coming days. Thanks Dario. |
We continued our investigations to understand if a repository uses Purpose:
References: How can this package be imported import MySQLdb
# or also
import _mysql_exceptions.py WMCore does not contain such import. It should be safe to drop py2-mysqldb from WMCore pacakges spec files |
We opened a similar issue in dmwm/CRABServer: dmwm/CRABServer#6228 |
I have not debugged this problem yet, but deployment of 1.4.2.pre7 - which no longer has the dependency on py2-mysqldb - failed when installing the database, see:
Dario, this is likely something that we have to discuss and see what our options are. |
And another problem while stopping the agent
|
Since we failed to get rid of this library, let me get it reassigned to you Dario ;-) |
So, I did test WMAgent without the meaning, we somehow still need it (not sure what for though).
Yes, I didn't even try to create the mysqldb package on python3 (thus py3-mysqldb.spec) because the developers say there is no support for python3. So there was no point in trying to build it and/or add it as a dependency for the python3 stack.
good catch! That means, our wmagentpy3.spec file has no dependency at all in
Yes, this is the first attempt I would chase. If it does not work, then we need to build a Just to wrap up these comments, here is what I would suggest to change (ordered):
What do you think? Have I missed anything? Thanks for this investigation, Todor. |
Hi @amaltaro thanks. This plan is exactly what I was having in my mind too. So we are on the same page here. But just to be sure: should we try the first two bullets separately/consequently or both together from the very first attempt? |
Just before I forget I need to log two additional steps which are a must in order to put the above plan in action.
Should be notified here too #10302
Which is crucial for the rest of the software to initiate and run. So we will have to fix the deployment script such that it also creates the following symlink:
Otherwise even if the [1]
FYI @amaltaro |
Todor, on your first bullet above. Another option would be to update the env.sh script, as mentioned in #10302 |
Hi @amaltaro. Yes, indeed this is an option, but it would require a manual intervention every time we deploy with |
Following the first scenario from this comment : #9805 (comment) and adding the So @amaltaro if you think we may merge those two PRs (in WMCore and cmsdist repositories) and close the current issue it would be great. The error mentioned in [2], we can follow in it's dedicated GH issue. [1]
[2]
|
Perfect! Thanks for spotting these issues and providing these fixes. However, I think those were pre-requirements to get you to be able to debug the actual issue reported in this issue. There has been quite some discussion here, but I'm still not sure whether we need As we have already discussed in other venues, when creating the wmagentpy3 spec file, So, the expected outcome of this issue will be either: Could you please investigate it? |
Oook then. Let me take a deep breath and I am about to dive deeper now :) |
Hi @amaltaro . So far, from what I am capable of tracing as imports in WMCore, I can not spot any direct or indirect import of the library
|
Wonderful! Thanks Todor and Dario. Just a note, if we face issues related with this in the future, we might want to directly add the "mariadb" dependency in wmagent.spec and drop "py2-mysqldb" one. This way we can compare things against something that we know is solid and working. |
Now that I managed to progress with the python3 environment, we seem to have missed one usage of this library (but I could be wrong as well...):
debugging |
@amaltaro |
It looks like we have a couple of optional drivers to use for MySQL which are supported in python3: I had a quick look and I guess I will try to build |
@amaltaro , in python if you need speed it is accomplished by Python-C bindings, therefore "pure" python is always slow with respect to Python-C binding implementation. Therefore, based on your use-case you need to decide to go with pure python or c-based implementation. The latter will always require gcc/glibc stack and therefore will be dependent on it. All high-performance tasks/libraries in python, e.g. NumPy, are basically Python-C binding to underlying C libraries. |
@amaltaro I am trying to create a pip build backage of |
@amaltaro here [1] is the PR providing a properly build package |
Hi again @amaltaro, Actually I figured out why it was having troubles. I had to put some more dependencies here and there - like making [2]
|
Actually I think the dependency I've put in p.s. I just checked and everything is deploying correctly upon reverting my previous commit in cmsdist |
Thanks for providing a swift fix to this issue, Todor. @todor-ivanov shall we close it? If so, please go ahead. |
I think we are safe to close it again. |
@todor-ivanov can you please open a new issue where we should have a basic benchmark between the two MySQL drivers that we have (for the different python stack)? It's important to assess what the performance impact might be with this new library, to avoid surprises on the FNAL nodes. |
Impact of the new feature
WMCore in general (and perhaps other services depending on it)
Is your feature request related to a problem? Please describe.
Yes, this package
MySQL-python
does not support python3. Actually, its last commit dates back from 2014...Describe the solution you'd like
Either completely deprecate this package from the COMP cmsdist repo:
https://github.com/cms-sw/cmsdist/blob/comp_gcc630/py2-mysqldb.spec
and update all the other specs depending on it. Or if this functionality is needed in WMCore (or other DMWM services), we need to find a library that supports python3 (and probably works still in python2), like
PyMySQL
:https://pypi.org/project/PyMySQL/
Describe alternatives you've considered
There is no alternative.
Additional context
Issue initially created here: cms-sw/cmsdist#6025
The text was updated successfully, but these errors were encountered: