Skip to content

BLD: Build shared c dependencies as a library #47081

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.

Sign up for GitHub

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

Closed
wants to merge 9 commits into from
Closed

BLD: Build shared c dependencies as a library #47081

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
13aa2f9
BLD: Build shared c dependencies as a library
lithomas1 May 21, 2022
6cd1449
Update action.yml
lithomas1 May 21, 2022
8325ff8
clean
lithomas1 May 24, 2022
9128cd5
Merge branch 'c-shared-library' of github-other.com:lithomas1/pandas …
lithomas1 May 24, 2022
9e09243
Merge branch 'main' into c-shared-library
lithomas1 Jun 16, 2022
b5545a7
change np_datetime.c references to be to the library (#16)
Dr-Irv Aug 21, 2022
21d121e
Merge branch 'main' into c-shared-library
lithomas1 Aug 21, 2022
8f92177
Merge branch 'main' into c-shared-library
lithomas1 Sep 10, 2022
681eacb
pre-commit
lithomas1 Sep 10, 2022
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
3 changes: 1 addition & 2 deletions .github/actions/build_pandas/action.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,4 @@ runs:
python -m pip install -e . --no-build-isolation --no-use-pep517 --no-index
shell: bash -el {0}
env:
# Cannot use parallel compilation on Windows, see https://github.com/pandas-dev/pandas/issues/30873
Copy link
Member

Choose a reason for hiding this comment

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

Unless this address's @simonjayhawkins #47081 (comment) I would prefer to keep this at N_JOBS: 1

Otherwise, I would suggest this be specific to the number of cores available on GHA hosted runners by OS: #47081 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Unless this address's @simonjayhawkins #47081 (comment)

I've needed to restart a few workflows this last week or so as they fail on the initial build, but it maybe once it is fixed that the initial build is successful, that subsequent incremental builds for commits in the range of the last couple of weeks could also be successful.

And it could also be that if this is a reoccurring problem that I can change the workflow to repeat the previous build step if the pandas import fails.

so I am happy that any comments/concerns are fully addressed before merging this.

N_JOBS: ${{ runner.os == 'Windows' && 1 || 2 }}
N_JOBS: 4
Copy link
Member

Choose a reason for hiding this comment

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

Might still be worth aligning this with the number of core on the GH hosted runners (2 Window/Linux, 3 MacOS): https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources=

61 changes: 44 additions & 17 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def build_extensions(self):
if _CYTHON_INSTALLED:
self.render_templates(_pxifiles)

self.run_command("build_clib")
super().build_extensions()


Expand Down Expand Up @@ -438,6 +439,33 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"pandas/_libs/tslibs/src/datetime/np_datetime_strings.h",
]

import sysconfig

ext_libraries = [
[
"np_datetime",
{
"sources": [
"pandas/_libs/tslibs/src/datetime/np_datetime.c",
"pandas/_libs/tslibs/src/datetime/np_datetime_strings.c",
],
"include_dirs": [
"pandas/_libs/tslibs/src/datetime",
sysconfig.get_path("include"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these get_path("include") calls? I think the compiler should handle that natively

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, the compiler was not able to find Python.h without this.

numpy.get_include(),
],
},
],
[
"tokenizer",
{
"sources": ["pandas/_libs/src/parser/tokenizer.c"],
"depends": ["pandas/_libs/src/parser/tokenizer.h"],
"include_dirs": [sysconfig.get_path("include")] + klib_include,
},
],
]

ext_data = {
"_libs.algos": {
"pyxfile": "_libs/algos",
Expand Down Expand Up @@ -472,7 +500,6 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"pyxfile": "_libs/lib",
"depends": lib_depends + tseries_depends,
"include": klib_include, # due to tokenizer import
"sources": ["pandas/_libs/src/parser/tokenizer.c"],
},
"_libs.missing": {"pyxfile": "_libs/missing", "depends": tseries_depends},
"_libs.parsers": {
Expand All @@ -482,8 +509,8 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"pandas/_libs/src/parser/tokenizer.h",
"pandas/_libs/src/parser/io.h",
],
"libraries": ["tokenizer"],
"sources": [
"pandas/_libs/src/parser/tokenizer.c",
"pandas/_libs/src/parser/io.c",
],
},
Expand All @@ -501,21 +528,18 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"_libs.tslibs.conversion": {
"pyxfile": "_libs/tslibs/conversion",
"depends": tseries_depends,
"sources": ["pandas/_libs/tslibs/src/datetime/np_datetime.c"],
"libraries": ["np_datetime"],
},
"_libs.tslibs.fields": {
"pyxfile": "_libs/tslibs/fields",
"depends": tseries_depends,
"sources": ["pandas/_libs/tslibs/src/datetime/np_datetime.c"],
"libraries": ["np_datetime"],
},
"_libs.tslibs.nattype": {"pyxfile": "_libs/tslibs/nattype"},
"_libs.tslibs.np_datetime": {
"pyxfile": "_libs/tslibs/np_datetime",
"depends": tseries_depends,
"sources": [
"pandas/_libs/tslibs/src/datetime/np_datetime.c",
"pandas/_libs/tslibs/src/datetime/np_datetime_strings.c",
],
"libraries": ["np_datetime"],
},
"_libs.tslibs.offsets": {
"pyxfile": "_libs/tslibs/offsets",
Expand All @@ -525,12 +549,12 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"pyxfile": "_libs/tslibs/parsing",
"include": klib_include,
"depends": ["pandas/_libs/src/parser/tokenizer.h"],
"sources": ["pandas/_libs/src/parser/tokenizer.c"],
"libraries": ["tokenizer"],
},
"_libs.tslibs.period": {
"pyxfile": "_libs/tslibs/period",
"depends": tseries_depends,
"sources": ["pandas/_libs/tslibs/src/datetime/np_datetime.c"],
"libraries": ["np_datetime"],
},
"_libs.tslibs.strptime": {
"pyxfile": "_libs/tslibs/strptime",
Expand All @@ -539,12 +563,12 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"_libs.tslibs.timedeltas": {
"pyxfile": "_libs/tslibs/timedeltas",
"depends": tseries_depends,
"sources": ["pandas/_libs/tslibs/src/datetime/np_datetime.c"],
"libraries": ["np_datetime"],
},
"_libs.tslibs.timestamps": {
"pyxfile": "_libs/tslibs/timestamps",
"depends": tseries_depends,
"sources": ["pandas/_libs/tslibs/src/datetime/np_datetime.c"],
"libraries": ["np_datetime"],
},
"_libs.tslibs.timezones": {"pyxfile": "_libs/tslibs/timezones"},
"_libs.tslibs.tzconversion": {
Expand Down Expand Up @@ -593,6 +617,7 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
depends=data.get("depends", []),
include_dirs=include,
language=data.get("language", "c"),
libraries=data.get("libraries", []),
define_macros=data.get("macros", macros),
extra_compile_args=extra_compile_args,
extra_link_args=extra_link_args,
Expand Down Expand Up @@ -626,17 +651,18 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
"pandas/_libs/src/ujson/lib/ultrajsonenc.c",
"pandas/_libs/src/ujson/lib/ultrajsondec.c",
]
+ [
"pandas/_libs/tslibs/src/datetime/np_datetime.c",
"pandas/_libs/tslibs/src/datetime/np_datetime_strings.c",
]
# + [
# "pandas/_libs/tslibs/src/datetime/np_datetime.c",
# "pandas/_libs/tslibs/src/datetime/np_datetime_strings.c",
# ]
),
include_dirs=[
"pandas/_libs/src/ujson/python",
"pandas/_libs/src/ujson/lib",
"pandas/_libs/src/datetime",
"pandas/_libs/tslibs/src/datetime",
numpy.get_include(),
],
libraries=["np_datetime"],
extra_compile_args=(["-D_GNU_SOURCE"] + extra_compile_args),
extra_link_args=extra_link_args,
define_macros=macros,
Expand All @@ -654,5 +680,6 @@ def srcpath(name=None, suffix=".pyx", subdir="src"):
setup(
version=versioneer.get_version(),
ext_modules=maybe_cythonize(extensions, compiler_directives=directives),
libraries=ext_libraries,
cmdclass=cmdclass,
)