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

ENH: implement cpython/time.pxd time, localtime #3767

Merged
merged 13 commits into from Jan 13, 2021

Conversation

jbrockmendel
Copy link
Contributor

closes #3733

Copy link
Contributor

@scoder scoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems useful. I left some comments.


from libc.stdint cimport int64_t

ctypedef int64_t _PyTime_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is defined in pytime.h (included from Python.h), so this should be declared extern (together with the functions below).

Cython/Includes/cpython/time.pxd Outdated Show resolved Hide resolved
tests/run/time_pxd.pyx Outdated Show resolved Hide resolved
Cython/Includes/cpython/time.pxd Outdated Show resolved Hide resolved
@jbrockmendel
Copy link
Contributor Author

Updated with suggested edits.

Out of curiosity, why the pattern of putting the assertions in doctests? Why not just assert in test_foo?

@TeamSpen210
Copy link
Contributor

The functions are compiled as Cython code, but the doctests are run in regular Python. This way we know they're doing the correct thing, since they'd be unaffected by any Cython bugs.

cdef extern from "pytime.h":
ctypedef _PyTime_t # alias for int64_t
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This declares it as object. :)

You still want to use int64_t here to tell Cython what it is, it's just that the cdef extern block lets Cython know that the actual declaration is in the header file and not here, and that it should not generate its own C declaration for it.

@jbrockmendel
Copy link
Contributor Author

Any chance the CI failures are unrelated?

@scoder
Copy link
Contributor

scoder commented Aug 12, 2020

They look related to me, e.g.:

     File "<doctest time_pxd.test_localtime[1]>", line 1, in <module>
        assert ltp.tm_year == ltc.tm_year
    AttributeError: 'dict' object has no attribute 'tm_year'
    AttributeError: 'dict' object has no attribute 'tm_mon'
    AttributeError: 'dict' object has no attribute 'tm_wday'".
    …

It's just the doctest that is wrong here.

@jbrockmendel
Copy link
Contributor Author

Let doctest do its work rather than calling "assert".

Trouble here is we don't get useful information when the test fails

@scoder
Copy link
Contributor

scoder commented Aug 12, 2020

Trouble here is we don't get useful information when the test fails

There's nothing less informative than a bare AssertionError, though. What we could do is to print the difference of the two, which should be 0. Or, rather, what I often do is
>>> x == y or (x, y)
to print the values if the assertion fails.

@scoder
Copy link
Contributor

scoder commented Aug 12, 2020

It is a bit unfortunate that there is an error case in localtime(). However unlikely it is, it now adds exception handling to the whole thing. :-/

@jbrockmendel
Copy link
Contributor Author

Looks like this is still producing compilation errors

======================================================================
ERROR: runTest (__main__.CythonRunTestCase)
compiling (c/cy2) and running time_pxd
----------------------------------------------------------------------
Traceback (most recent call last):
  File "runtests.py", line 1289, in run
    ext_so_path = self.runCompileTest()
  File "runtests.py", line 958, in runCompileTest
    self.test_directory, self.expect_errors, self.expect_warnings, self.annotate)
  File "runtests.py", line 1223, in compile
    so_path = self.run_distutils(test_directory, module, workdir, incdir)
  File "runtests.py", line 1144, in run_distutils
    build_extension.run()
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 340, in run
    self.build_extensions()
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 449, in build_extensions
    self.build_extension(ext)
  File "runtests.py", line 520, in build_extension
    _build_ext.build_extension(self, ext)
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 499, in build_extension
    depends=ext.depends)
  File "/Users/travis/miniconda/lib/python2.7/distutils/ccompiler.py", line 574, in compile
    self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)
  File "/Users/travis/miniconda/lib/python2.7/distutils/unixccompiler.py", line 124, in _compile
    raise CompileError, msg
CompileError: command 'clang' failed with exit status 1
======================================================================
ERROR: runTest (__main__.CythonRunTestCase)
compiling (cpp/cy2) and running time_pxd
----------------------------------------------------------------------
Traceback (most recent call last):
  File "runtests.py", line 1289, in run
    ext_so_path = self.runCompileTest()
  File "runtests.py", line 958, in runCompileTest
    self.test_directory, self.expect_errors, self.expect_warnings, self.annotate)
  File "runtests.py", line 1223, in compile
    so_path = self.run_distutils(test_directory, module, workdir, incdir)
  File "runtests.py", line 1144, in run_distutils
    build_extension.run()
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 340, in run
    self.build_extensions()
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 449, in build_extensions
    self.build_extension(ext)
  File "runtests.py", line 520, in build_extension
    _build_ext.build_extension(self, ext)
  File "/Users/travis/miniconda/lib/python2.7/distutils/command/build_ext.py", line 499, in build_extension
    depends=ext.depends)
  File "/Users/travis/miniconda/lib/python2.7/distutils/ccompiler.py", line 574, in compile
    self._compile(obj, src, ext, cc_args, extra_postargs, pp_opts)
  File "/Users/travis/miniconda/lib/python2.7/distutils/unixccompiler.py", line 124, in _compile
    raise CompileError, msg
CompileError: command 'clang' failed with exit status 1

@scoder
Copy link
Contributor

scoder commented Aug 14, 2020

Those are test failures, because tm_wday also differs. Look at what CPython does:
https://github.com/python/cpython/blob/bb0b08540cc93e56f3f1bde1b39ce086d9e35fe1/Modules/timemodule.c#L397-L405

Failed example:
    ltp.tm_wday == ltc['tm_wday']  or (ltp.tm_wday, ltc['tm_wday'])
Expected:
    True
Got:
    (2, 4)

@scoder
Copy link
Contributor

scoder commented Aug 14, 2020

Looks like this is still producing compilation errors

Ah, those are in the Py2.7 builds. Looks like pytime.h is a pure Py3 feature.

Edit: Py3.5+, actually, the way you use it.

@jbrockmendel
Copy link
Contributor Author

Ah, those are in the Py2.7 builds. Looks like pytime.h is a pure Py3 feature.

So I guess disable the code/tests for >py35?

@jbrockmendel
Copy link
Contributor Author

Rebased yesterday. Looking at test logs, none of the failures look related to this PR (though the logs are huge, so worth double-checking)

Am I right in thinking that a/the barrier to releasing 3.0 is getting the CI passing?

@da-woods
Copy link
Contributor

da-woods commented Jan 9, 2021

Rebased yesterday. Looking at test logs, none of the failures look related to this PR (though the logs are huge, so worth double-checking)

I think you're right - the failures are all existing ones, not introduced by your PR

Am I right in thinking that a/the barrier to releasing 3.0 is getting the CI passing?

I don't think it's the only barrier (I'm not sure what list of features is currently targetted for 3.0). I'm sure fixes to get the CI passing would be well-received though. I think the C++/pythran issues are a genuine bug (not sure if it's a bug in Pythran or Cython though...), the PyPy issues are a bug/omission in PyPy that's now been fixed (but the fixed version probably hasn't propagated to the CI yet), and the issues on older versions of Python 3 are probably just CI issues. The CI may well be migrated in the near future though

@robertwb
Copy link
Contributor

The CI failures do look unrelated to this PR.

@robertwb robertwb merged commit 5d7d0db into cython:master Jan 13, 2021
@jbrockmendel jbrockmendel deleted the ctime branch January 13, 2021 15:37
da-woods added a commit to da-woods/cython that referenced this pull request Feb 16, 2021
It uses features that are unavailable thus always fails

Test was introduced in cython#3767
@scoder scoder added this to the 3.0 milestone Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ENH: cpython.time to expose stdlib time.localtime etc
5 participants