Pig
  1. Pig
  2. PIG-2657

Print warning if using wrong jython version

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: 0.12.0
    • Component/s: None
    • Labels:
    • Release Note:
      Log a warning if jython version is not 2.5.0

      Description

      Hi,

      It would be good if Pig would print a warning (or refuse to run) if you are using an unsupported version of jython. I spent a couple of hours before figuring out that you had to use 2.5.0. I've seen posts indicating that others have run into this problem as well.

      Might write up a patch if others agree this is an issue.

      1. PIG-2657.4.patch
        5 kB
        Johnny Zhang
      2. PIG-2657.3.patch
        5 kB
        Johnny Zhang
      3. PIG-2657.2.patch
        5 kB
        Fabian Alenius
      4. PIG-2657.1.patch
        1 kB
        Fabian Alenius

        Activity

        Hide
        Cheolsoo Park added a comment -

        Closing it as a 'won't fix'.

        Show
        Cheolsoo Park added a comment - Closing it as a 'won't fix'.
        Hide
        Cheolsoo Park added a comment -

        I'd like to close this jira as a 'won't fix'. Please let me if anyone has objections.

        Show
        Cheolsoo Park added a comment - I'd like to close this jira as a 'won't fix'. Please let me if anyone has objections.
        Hide
        Johnny Zhang added a comment -

        Cheolsoo Park, agree with that. Unless we can find a better way to detect Jython version, we should not push it in.

        Show
        Johnny Zhang added a comment - Cheolsoo Park , agree with that. Unless we can find a better way to detect Jython version, we should not push it in.
        Hide
        Cheolsoo Park added a comment -

        Hi Johnny,

        Thank you very much for your time and effort. But I don't think that you tested it correctly. I think that jython 2.5.0 is present somewhere in your classpath, and that's why you're seeing that message. Here is my test using jython 2.2.1:

        • Commented out the following lines in bin/pig so that jython 2.5.x is not present in my classpath. If you don't do this, you will end up with 2 versions of Jython in classpath:
          #JYTHON_JAR=`echo ${PIG_HOME}/lib/jython*.jar`
          
          #if [ -z "$JYTHON_JAR" ]; then
          #    JYTHON_JAR=`echo $PIG_HOME/build/ivy/lib/Pig/jython*.jar`
          #    if [ -n "$JYTHON_JAR" ]; then
          #        CLASSPATH=${CLASSPATH}:$JYTHON_JAR
          #    fi
          #fi
          
        • Set PIG_CLASSPATH=/usr/share/java/jython-2.2.1.jar.
        • Ran a Python UDF.

        The result for me is a NoClassDefFoundError as follows:

        ERROR 2998: Unhandled internal error. org/python/Version
        
        java.lang.NoClassDefFoundError: org/python/Version
        	at org.apache.pig.scripting.jython.JythonScriptEngine$Interpreter.<clinit>(JythonScriptEngine.java:92)
        

        The reason is because the org.python.Version class is introduced in Jython 2.5.x, and thus doesn't exist in Jython 2.2.x:
        http://grepcode.com/project/repo1.maven.org/maven2/org.python/jython/

        In fact, this gives me a pause. By doing this, we're going to introduce a backward incompatibility with Jython releases prior to 2.5.x. For example, there may be some Python UDFs that used to work with Jython 2.2.x but will no longer work because of this change. After all, I am not sure if it is worthwhile to fix. Thoughts?

        Show
        Cheolsoo Park added a comment - Hi Johnny, Thank you very much for your time and effort. But I don't think that you tested it correctly. I think that jython 2.5.0 is present somewhere in your classpath, and that's why you're seeing that message. Here is my test using jython 2.2.1: Commented out the following lines in bin/pig so that jython 2.5.x is not present in my classpath. If you don't do this, you will end up with 2 versions of Jython in classpath: #JYTHON_JAR=`echo ${PIG_HOME}/lib/jython*.jar` # if [ -z "$JYTHON_JAR" ]; then # JYTHON_JAR=`echo $PIG_HOME/build/ivy/lib/Pig/jython*.jar` # if [ -n "$JYTHON_JAR" ]; then # CLASSPATH=${CLASSPATH}:$JYTHON_JAR # fi #fi Set PIG_CLASSPATH=/usr/share/java/jython-2.2.1.jar . Ran a Python UDF. The result for me is a NoClassDefFoundError as follows: ERROR 2998: Unhandled internal error. org/python/Version java.lang.NoClassDefFoundError: org/python/Version at org.apache.pig.scripting.jython.JythonScriptEngine$Interpreter.<clinit>(JythonScriptEngine.java:92) The reason is because the org.python.Version class is introduced in Jython 2.5.x, and thus doesn't exist in Jython 2.2.x: http://grepcode.com/project/repo1.maven.org/maven2/org.python/jython/ In fact, this gives me a pause. By doing this, we're going to introduce a backward incompatibility with Jython releases prior to 2.5.x. For example, there may be some Python UDFs that used to work with Jython 2.2.x but will no longer work because of this change. After all, I am not sure if it is worthwhile to fix. Thoughts?
        Hide
        Johnny Zhang added a comment -

        Cheolsoo Park, thanks for the comments, 'PIG-2657.4.patch' is the patch based on your comments, I did the positive test and this the warning message printed

        2012-11-12 23:45:27,753 [main] WARN  org.apache.pig.scripting.jython.JythonScriptEngine - Pig is tested with 2.5.2, so it may not work with 2.5.0.
        

        the 2.5.2 is the version string define in the /pig/ivy/libraries.properties in trunk, so it is correct. But 2.5.0 seems not the jython version in my env. Mine seems 2.2.1

        jython --version
        Jython 2.2.1 on java1.6.0_31
        

        what do you think about it ?

        Show
        Johnny Zhang added a comment - Cheolsoo Park , thanks for the comments, ' PIG-2657 .4.patch' is the patch based on your comments, I did the positive test and this the warning message printed 2012-11-12 23:45:27,753 [main] WARN org.apache.pig.scripting.jython.JythonScriptEngine - Pig is tested with 2.5.2, so it may not work with 2.5.0. the 2.5.2 is the version string define in the /pig/ivy/libraries.properties in trunk, so it is correct. But 2.5.0 seems not the jython version in my env. Mine seems 2.2.1 jython --version Jython 2.2.1 on java1.6.0_31 what do you think about it ?
        Hide
        Cheolsoo Park added a comment -

        Hi Johnny,

        Thank you very much for the patch. I have a few more comments:

        • I think that you have to swap Version.PY_VERSION and jythonVersion.
        • Looking at JythonScriptEngine.java after apply your patch, I see no reason why we nest try-catch blocks here. Can you please move the inner try-catch block to outside the outer one? In addition, can you make it to catch an IOException instead of an Exception since that's specifically what is thrown by JarFile? Do you agree?
        • Please remove tabs in build.xml.
        Show
        Cheolsoo Park added a comment - Hi Johnny, Thank you very much for the patch. I have a few more comments: I think that you have to swap Version.PY_VERSION and jythonVersion . Looking at JythonScriptEngine.java after apply your patch, I see no reason why we nest try-catch blocks here. Can you please move the inner try-catch block to outside the outer one? In addition, can you make it to catch an IOException instead of an Exception since that's specifically what is thrown by JarFile ? Do you agree? Please remove tabs in build.xml .
        Hide
        Johnny Zhang added a comment -

        Cheolsoo Park, here is the new patch based on your comments.

        Show
        Johnny Zhang added a comment - Cheolsoo Park , here is the new patch based on your comments.
        Hide
        Cheolsoo Park added a comment -

        Additional comments:

        • Please remove tabs. Use 4 space instead.
        • The property jython.version shouldn't be hard-coded in build.xml. It is automatically loaded from ivy/libraries.properties at compile-time, so no reason to define it.
        • The jython.version attribute shouldn't be embedded in pigunit.jar. It's useful only in pig.jar and pig-withouthadoop.jar.
        • Regarding the log message, why don't we print a message like "Pig is tested with $ {jython.version}

          , so it may not work with $

          {runtime.jython.version}

          "? I think that this is flexible and informative at the same time.

        Thanks!

        Show
        Cheolsoo Park added a comment - Additional comments: Please remove tabs. Use 4 space instead. The property jython.version shouldn't be hard-coded in build.xml . It is automatically loaded from ivy/libraries.properties at compile-time, so no reason to define it. The jython.version attribute shouldn't be embedded in pigunit.jar . It's useful only in pig.jar and pig-withouthadoop.jar . Regarding the log message, why don't we print a message like "Pig is tested with $ {jython.version} , so it may not work with $ {runtime.jython.version} "? I think that this is flexible and informative at the same time. Thanks!
        Hide
        Olga Natkovich added a comment -

        Moving to 0.12 based on Rohini's recommendation. Please, move back if you feel it needs to make it to 0.11

        Show
        Olga Natkovich added a comment - Moving to 0.12 based on Rohini's recommendation. Please, move back if you feel it needs to make it to 0.11
        Hide
        Rohini Palaniswamy added a comment -

        Fabian Alenius,
        Checking for a fixed version is not helpful when a range of versions is not supported. Currently trunk is packaged with 2.5.2 and this would give a warning for that. If there is a problem with lower version of jython, then we can print a warning saying versions lesser than 2.5.0 are not supported. If we see problem with higher versions of jython, then we need to fix pig to work with them. Can you tell what version of python gives error for you?

        And I feel this is more suitable for 0.12 now that 0.11 is branched. If no one has objections, I will move the Fix Version to 0.12.

        Show
        Rohini Palaniswamy added a comment - Fabian Alenius , Checking for a fixed version is not helpful when a range of versions is not supported. Currently trunk is packaged with 2.5.2 and this would give a warning for that. If there is a problem with lower version of jython, then we can print a warning saying versions lesser than 2.5.0 are not supported. If we see problem with higher versions of jython, then we need to fix pig to work with them. Can you tell what version of python gives error for you? And I feel this is more suitable for 0.12 now that 0.11 is branched. If no one has objections, I will move the Fix Version to 0.12.
        Hide
        Daniel Dai added a comment -

        I probably get confused with PIG-2665. The patch looks good, +1.

        Show
        Daniel Dai added a comment - I probably get confused with PIG-2665 . The patch looks good, +1.
        Hide
        Jonathan Coveney added a comment -

        Bumping again. Gotta love that JIRA report.

        Show
        Jonathan Coveney added a comment - Bumping again. Gotta love that JIRA report.
        Hide
        Jonathan Coveney added a comment -

        Bump. Daniel, I'm also unsure of what you mean.

        Show
        Jonathan Coveney added a comment - Bump. Daniel, I'm also unsure of what you mean.
        Hide
        Fabian Alenius added a comment -

        Ah, hm will that mean the wrong version can not be loaded?

        Show
        Fabian Alenius added a comment - Ah, hm will that mean the wrong version can not be loaded?
        Hide
        Daniel Dai added a comment -

        Hi, Fabian, what I mean is update ivy.xml to use jython-standalone instead of jython.

        Show
        Daniel Dai added a comment - Hi, Fabian, what I mean is update ivy.xml to use jython-standalone instead of jython.
        Hide
        Fabian Alenius added a comment -

        I've added another patch that works the way Gianmarco described. I.e. storing the jython version in the build.xml file and loading it from MANIFEST.MF.

        Show
        Fabian Alenius added a comment - I've added another patch that works the way Gianmarco described. I.e. storing the jython version in the build.xml file and loading it from MANIFEST.MF.
        Hide
        Fabian Alenius added a comment -

        New version where the current jython version is stored in the build.xml file.

        Show
        Fabian Alenius added a comment - New version where the current jython version is stored in the build.xml file.
        Hide
        Fabian Alenius added a comment -

        Daniel:

        How should this be done? Can you explain in more detail what you mean?

        Show
        Fabian Alenius added a comment - Daniel: How should this be done? Can you explain in more detail what you mean?
        Hide
        Daniel Dai added a comment -

        We shall solve it by pulling the right jython-standalone.jar.

        Show
        Daniel Dai added a comment - We shall solve it by pulling the right jython-standalone.jar.
        Hide
        Gianmarco De Francisci Morales added a comment -

        Hi,
        thanks for sending a patch.
        I have one concern, on which I would like advice from the community.
        Hardcoding version numbers in the source is not maintainable in the long run in my opinion (we will forget about this when we upgrade jython).
        Is there a way to programmatically and portably get the version number?
        It would be easy to read the ivy/library.properties file and get the jython.version property, however this file is not bundled with pig.jar.
        My suggestion would be to put this information in the MANIFEST.MF, which is created by ant.
        This way it is easily recoverable from within the jar and it is easy to put there because the version number is available during the build process.

        Thoughts?

        Show
        Gianmarco De Francisci Morales added a comment - Hi, thanks for sending a patch. I have one concern, on which I would like advice from the community. Hardcoding version numbers in the source is not maintainable in the long run in my opinion (we will forget about this when we upgrade jython). Is there a way to programmatically and portably get the version number? It would be easy to read the ivy/library.properties file and get the jython.version property, however this file is not bundled with pig.jar. My suggestion would be to put this information in the MANIFEST.MF, which is created by ant. This way it is easily recoverable from within the jar and it is easy to put there because the version number is available during the build process. Thoughts?
        Hide
        Fabian Alenius added a comment -

        Log a warning if jython version is not 2.5.0

        Show
        Fabian Alenius added a comment - Log a warning if jython version is not 2.5.0
        Hide
        Fabian Alenius added a comment -

        Log a warning if jython version is not 2.5.0

        Tested against trunk.

        Show
        Fabian Alenius added a comment - Log a warning if jython version is not 2.5.0 Tested against trunk.
        Hide
        Fabian Alenius added a comment -

        Log a warning if jython version is not 2.5.0

        Show
        Fabian Alenius added a comment - Log a warning if jython version is not 2.5.0
        Hide
        Gianmarco De Francisci Morales added a comment -

        It sounds sensible to print a warning.

        Please go ahead if you want to send a patch.

        Show
        Gianmarco De Francisci Morales added a comment - It sounds sensible to print a warning. Please go ahead if you want to send a patch.

          People

          • Assignee:
            Unassigned
            Reporter:
            Fabian Alenius
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development