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

Replace flake8 with ruff & cython-lint #168

Merged
merged 2 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,15 @@ repos:
hooks:
- id: black
files: python/.*
- repo: https://github.com/PyCQA/flake8
rev: 5.0.4
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.0.278
hooks:
- id: flake8
args: ["--config=python/.flake8"]
files: python/.*\.py$
types: [python]
- id: flake8
args: ["--config=python/.flake8.cython"]
types: [cython]
additional_dependencies: ["flake8-force"]
- id: ruff
files: python/.*$
- repo: https://github.com/MarcoGorelli/cython-lint
rev: v0.15.0
hooks:
- id: cython-lint
- repo: https://github.com/pre-commit/mirrors-clang-format
rev: v16.0.6
hooks:
Expand Down
13 changes: 0 additions & 13 deletions python/.flake8

This file was deleted.

31 changes: 0 additions & 31 deletions python/.flake8.cython

This file was deleted.

13 changes: 13 additions & 0 deletions python/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,16 @@ cmake.minimum-version = "3.26.4"
ninja.make-fallback = true
sdist.reproducible = true
wheel.packages = ["ucxx"]

[tool.ruff]
select = ["E", "F", "W"]
ignore = [
# whitespace before :
"E203",
]
fixable = ["ALL"]
exclude = [
# TODO: Remove this in a follow-up where we fix __all__.
"__init__.py",
]
line-length = 88
59 changes: 36 additions & 23 deletions python/ucxx/_lib/libucxx.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ from cython.operator cimport dereference as deref
from libc.stdint cimport uintptr_t
from libcpp cimport nullptr
from libcpp.functional cimport function
from libcpp.map cimport map as cpp_map
from libcpp.memory cimport (
dynamic_pointer_cast,
make_shared,
Expand All @@ -24,15 +23,13 @@ from libcpp.memory cimport (
unique_ptr,
)
from libcpp.string cimport string
from libcpp.unordered_map cimport unordered_map
from libcpp.utility cimport move
from libcpp.vector cimport vector

import numpy as np

from rmm._lib.device_buffer cimport DeviceBuffer

from . cimport ucxx_api
from .arr cimport Array
from .ucxx_api cimport *

Expand Down Expand Up @@ -290,7 +287,7 @@ cdef class UCXContext():

def __init__(
self,
config_dict={},
config_dict=None,
feature_flags=(
Feature.TAG,
Feature.WAKEUP,
Expand All @@ -302,6 +299,9 @@ cdef class UCXContext():
cdef ConfigMap cpp_config_in, cpp_config_out
cdef dict context_config

if config_dict is None:
config_dict = {}

Comment on lines +302 to +304
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, is this done automatically by the linter or did you have to write manually @charlesbluca ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was done manually - does this seem like a suitable alternative to the mutable default arg?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, that's in line with what we do in other places. I was primarily curious if the linter was that smart or you actually intervened manually. 🙂

for k, v in config_dict.items():
cpp_config_in[k.encode("utf-8")] = v.encode("utf-8")
cdef uint64_t feature_flags_uint = functools.reduce(
Expand Down Expand Up @@ -436,8 +436,6 @@ cdef class UCXAddress():
return bytes(self._string)

def __getbuffer__(self, Py_buffer *buffer, int flags):
cdef Address* address_ptr = self._address.get()

if bool(flags & PyBUF_WRITABLE):
raise BufferError("Requested writable view on readonly data")
buffer.buf = self._handle
Expand Down Expand Up @@ -508,12 +506,16 @@ cdef class UCXWorker():
ucxx_enable_delayed_submission,
ucxx_enable_python_future,
)
self._enable_delayed_submission = self._worker.get().isDelayedRequestSubmissionEnabled()
self._enable_delayed_submission = (
self._worker.get().isDelayedRequestSubmissionEnabled()
)
self._enable_python_future = self._worker.get().isFutureEnabled()

if self._context_feature_flags & UCP_FEATURE_AM:
rmm_am_allocator = <AmAllocatorType>(&_rmm_am_allocator)
self._worker.get().registerAmAllocator(UCS_MEMORY_TYPE_CUDA, rmm_am_allocator)
self._worker.get().registerAmAllocator(
UCS_MEMORY_TYPE_CUDA, rmm_am_allocator
)

def __dealloc__(self):
with nogil:
Expand Down Expand Up @@ -612,7 +614,9 @@ cdef class UCXWorker():
cdef int ucxx_epoll_timeout = epoll_timeout

with nogil:
self._worker.get().startProgressThread(polling_mode, epoll_timeout=ucxx_epoll_timeout)
self._worker.get().startProgressThread(
polling_mode, epoll_timeout=ucxx_epoll_timeout
)

def stop_progress_thread(self):
with nogil:
Expand Down Expand Up @@ -696,14 +700,19 @@ cdef class UCXWorker():

def is_python_future_enabled(self):
warnings.warn(
"UCXWorker.is_python_future_enabled() is deprecated and will soon be removed, "
"use the UCXWorker.enable_python_future property instead",
"UCXWorker.is_python_future_enabled() is deprecated and will soon be "
"removed, use the UCXWorker.enable_python_future property instead",
FutureWarning,
stacklevel=2,
)
return self.enable_python_future

def tag_recv(self, Array arr, tag: UCXXTagMask, tag_mask: UCXXTagMask = UCXXTagMaskFull):
def tag_recv(
self,
Array arr,
tag: UCXXTagMask,
tag_mask: UCXXTagMask = UCXXTagMaskFull
):
if not isinstance(tag, UCXXTag):
raise TypeError(f"The `tag` object must be of type {UCXXTag}")
if not isinstance(tag_mask, UCXXTagMask):
Expand Down Expand Up @@ -910,7 +919,6 @@ cdef class UCXBufferRequests:
tuple _requests

def __init__(self, uintptr_t unique_ptr_buffer_requests, bint enable_python_future):
cdef RequestTagMulti ucxx_buffer_requests
self._enable_python_future = enable_python_future
self._completed = False
self._requests = tuple()
Expand Down Expand Up @@ -1008,8 +1016,8 @@ cdef class UCXBufferRequests:

def is_completed_all(self):
warnings.warn(
"UCXBufferRequests.is_completed_all() is deprecated and will soon be removed, "
"use the UCXBufferRequests.all_completed property instead",
"UCXBufferRequests.is_completed_all() is deprecated and will soon be "
"removed, use the UCXBufferRequests.all_completed property instead",
FutureWarning,
stacklevel=2,
)
Expand Down Expand Up @@ -1062,8 +1070,8 @@ cdef class UCXBufferRequests:

def get_py_buffers(self):
warnings.warn(
"UCXBufferRequests.get_py_buffers() is deprecated and will soon be removed, "
"use the UCXBufferRequests.py_buffers property instead",
"UCXBufferRequests.get_py_buffers() is deprecated and will soon be "
"removed, use the UCXBufferRequests.py_buffers property instead",
FutureWarning,
stacklevel=2,
)
Expand Down Expand Up @@ -1369,7 +1377,12 @@ cdef class UCXEndpoint():

return UCXRequest(<uintptr_t><void*>&req, self._enable_python_future)

def tag_recv(self, Array arr, tag: UCXXTagMask, tag_mask: UCXXTagMask=UCXXTagMaskFull):
def tag_recv(
self,
Array arr,
tag: UCXXTagMask,
tag_mask: UCXXTagMask = UCXXTagMaskFull
):
if not isinstance(tag, UCXXTag):
raise TypeError(f"The `tag` object must be of type {UCXXTag}")
if not isinstance(tag_mask, UCXXTagMask):
Expand Down Expand Up @@ -1419,8 +1432,8 @@ cdef class UCXEndpoint():
raise ValueError(
"UCX is not configured with CUDA support, please ensure that the "
"available UCX on your environment is built against CUDA and that "
"`cuda` or `cuda_copy` are present in `UCX_TLS` or that it is using "
"the default `UCX_TLS=all`."
"`cuda` or `cuda_copy` are present in `UCX_TLS` or that it is "
"using the default `UCX_TLS=all`."
)

for arr in arrays:
Expand All @@ -1441,7 +1454,7 @@ cdef class UCXEndpoint():
<uintptr_t><void*>&ucxx_buffer_requests, self._enable_python_future,
)

def tag_recv_multi(self, tag: UCXXTagMask, tag_mask: UCXXTagMask=UCXXTagMaskFull):
def tag_recv_multi(self, tag: UCXXTagMask, tag_mask: UCXXTagMask = UCXXTagMaskFull):
if not isinstance(tag, UCXXTag):
raise TypeError(f"The `tag` object must be of type {UCXXTag}")
if not isinstance(tag_mask, UCXXTagMask):
Expand Down Expand Up @@ -1606,8 +1619,8 @@ cdef class UCXListener():

def is_python_future_enabled(self):
warnings.warn(
"UCXListener.is_python_future_enabled() is deprecated and will soon be removed, "
"use the UCXListener.enable_python_future property instead",
"UCXListener.is_python_future_enabled() is deprecated and will soon be "
"removed, use the UCXListener.enable_python_future property instead",
FutureWarning,
stacklevel=2,
)
Expand Down
30 changes: 17 additions & 13 deletions python/ucxx/_lib/ucxx_api.pxd
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
from posix cimport fcntl

cimport numpy as np
from libc.stdint cimport int64_t, uint16_t, uint64_t # noqa: E402
from libcpp cimport bool as cpp_bool # noqa: E402
from libcpp.functional cimport function # noqa: E402
from libcpp.memory cimport shared_ptr, unique_ptr # noqa: E402
from libcpp.string cimport string # noqa: E402
from libcpp.unordered_map cimport ( # noqa: E402
unordered_map as cpp_unordered_map,
)
from libcpp.vector cimport vector # noqa: E402
from libc.stdint cimport int64_t, uint16_t, uint64_t
from libcpp cimport bool as cpp_bool
from libcpp.functional cimport function
from libcpp.memory cimport shared_ptr, unique_ptr
from libcpp.string cimport string
from libcpp.unordered_map cimport unordered_map as cpp_unordered_map
from libcpp.vector cimport vector


cdef extern from "Python.h" nogil:
Expand Down Expand Up @@ -186,8 +184,9 @@ cdef extern from "<ucxx/api.h>" namespace "ucxx" nogil:
# ctypedef Tag CppTag
# ctypedef TagMask CppTagMask

# Using function[Buffer] here doesn't seem possible due to Cython bugs/limitations. The
# workaround is to use a raw C function pointer and let it be parsed by the compiler.
# Using function[Buffer] here doesn't seem possible due to Cython bugs/limitations.
# The workaround is to use a raw C function pointer and let it be parsed by the
# compiler.
# See https://github.com/cython/cython/issues/2041 and
# https://github.com/cython/cython/issues/3193
ctypedef shared_ptr[Buffer] (*AmAllocatorType)(size_t)
Expand Down Expand Up @@ -265,13 +264,18 @@ cdef extern from "<ucxx/api.h>" namespace "ucxx" nogil:
bint isDelayedRequestSubmissionEnabled() const
bint isFutureEnabled() const
bint amProbe(ucp_ep_h) const
void registerAmAllocator(ucs_memory_type_t memoryType, AmAllocatorType allocator)
void registerAmAllocator(
ucs_memory_type_t memoryType, AmAllocatorType allocator
)

cdef cppclass Endpoint(Component):
ucp_ep_h getHandle()
void close(uint64_t period, uint64_t maxAttempts)
shared_ptr[Request] amSend(
void* buffer, size_t length, ucs_memory_type_t memory_type, bint enable_python_future
void* buffer,
size_t length,
ucs_memory_type_t memory_type,
bint enable_python_future
) except +raise_py_error
shared_ptr[Request] amRecv(
bint enable_python_future
Expand Down
1 change: 0 additions & 1 deletion python/ucxx/examples/python_future_task_app.pyx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ from cpython.ref cimport PyObject
from libcpp.memory cimport make_unique, unique_ptr
from libcpp.utility cimport move

from . cimport python_future_task_api
from .python_future_task_api cimport *


Expand Down