Build lib separately (shared on unix; stay static on win) #33

h-vetinari · 2022-12-10T13:43:01Z

Probably time to give up this hopeless battle for the shared windows libs

Closes #32
Closes #30
Closes #26
Closes #25

conda-forge-linter · 2022-12-10T13:43:05Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

and incorporating lots of experience from conda-forge#26 & conda-forge#30

developed in conda-forge#26

…nda-forge-pinning 2022.12.11.21.21.05

h-vetinari

Hey @xhochy

I finally cracked this one. 🥳
The main change to the previous attempts (#26, #30, #32) was that I gave up on shared libraries on windows - which, honestly, I feel I should have done months ago. Protobuf+windows+shared is just an insane effort (also seen in grpc, etc.), so I don't feel too bad about that.

This package comes with some binaries as well, which are currently placed with libsentencepiece (because I didn't want to do further CMake surgery), making it akin to libprotobuf carrying protoc. Are you strongly against that? If yes, how would you name an output for the binaries?

Would appreciate your review. 🙃

Likewise for you all, @isuruf @hmaarrfk @carterbox

h-vetinari · 2022-12-12T06:17:56Z

recipe/meta.yaml

+        # binaries
+        {% for each_bin in ["decode", "encode", "export_vocab", "normalize", "train"] %}
+        - spm_{{ each_bin }} --help
+        {% endfor %}


See here for binaries

h-vetinari · 2022-12-12T06:31:33Z

recipe/patches/0006-stop-pretending-sp-glue-code-belongs-in-third_party-.patch

+-#define ABSL_FLAG(Type, name, defautl_value, help) \
+-  absl::Flag<Type> FLAGS_##name(#name, #Type, help, defautl_value);
+#define STPC_FLAG(Type, name, default_value, help) \
+  sentencepiece::Flag<Type> FLAGS_##name(#name, #Type, help, default_value);


There might be ways to avoid patch 6, but sentencepiece definitely does not use abseil in the way that's prescribed by abseil - it's already heavily blurring the lines of third_party with sentencepiece-specific glue code, but redefining the ABSL_FLAG macro was the last straw at the time that just made me rip out this whole piece and place it somewhere else. Perhaps there's a less nuclear way.

recipe/build-lib.sh

recipe/patches/0004-use-C-17.patch

xhochy · 2022-12-12T09:51:07Z

recipe/patches/0005-ACTUALLY-use-external-absl.patch

@@ -0,0 +1,884 @@
+From 9f225f6f83ccea1909974ee95da9b724d017dad3 Mon Sep 17 00:00:00 2001


Will you be able to maintain this patch "manually"? Seems like the changes here could be better persisted as a well-formed sed command.

I'm planning to get some of these changes upstream in some form...

xhochy · 2022-12-12T10:47:37Z

Looks good in general. The comments were mostly there to hint at places where you could ease your maintenance.

hmaarrfk · 2022-12-12T13:09:23Z

given you got xonchy's attention, i think you are in good hands! Ill refrain from further comments.

carterbox · 2022-12-12T16:47:01Z

This package comes with some binaries as well, which are currently placed with libsentencepiece (because I didn't want to do further CMake surgery)

In most cases, you can build the package without installing it to PREFIX. Then manually install components to PREFIX using a bash script for each recipe output. Editing CMake is typically only required if you are trying to separate libs into multiple outputs because you have to separate the CMake config files (but binaries don't have config files).

h-vetinari · 2022-12-12T22:09:39Z

@carterbox
Thanks for taking a look. Yes, it would be possible to hard remove $PREFIX/bin/spm_* also in build-lib.sh, but that still leaves the question of how to name the output with only the binaries, and whether that's a net value add (i.e. the python bindings should probably still depend on whatever contains the binaries, otherwise people will be caught out by their absence when just installing sentencepiece).

carterbox · 2022-12-12T23:51:12Z

how to name the output with only the binaries

It could be named spm or sentencepiece-spm. The python package becomes sentencepiece-python and sentencepiece is a metapackage that includes everything.

whether that's a net value add

Can't comment on value add. Not familiar enough with the sizes of the binaries or use patterns of this package.

h-vetinari · 2022-12-12T23:55:04Z

It could be named spm or sentencepiece-spm. The python package becomes sentencepiece-python and sentencepiece is a metapackage that includes everything.

Not a bad idea IMO. Thoughts @xhochy?

…ackage

h-vetinari · 2022-12-14T11:43:01Z

@carterbox, I implemented your idea, PTAL. Still interested if you'd also have some thoughts on this approach @xhochy :)

recipe/build-lib.sh

carterbox · 2022-12-16T01:15:19Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2022.12.15.19.46.12

recipe/meta.yaml

carterbox · 2022-12-16T01:40:30Z

OK. No more suggestions after that last one. Looks very good! Great job, @h-vetinari.

Co-authored-by: Daniel Ching <carterbox@users.noreply.github.com>

setu4993

Great job, @h-vetinari! Thanks for being persistent.

h-vetinari requested review from ndmaxar, oblute, rluria14 and setu4993 as code owners December 10, 2022 13:43

h-vetinari force-pushed the lib branch 2 times, most recently from 72db7e0 to 0c5db76 Compare December 12, 2022 05:44

h-vetinari added 16 commits December 12, 2022 17:02

break out libsentencepiece

8370544

and incorporating lots of experience from conda-forge#26 & conda-forge#30

libsentencepiece: add tests & run-export

f846795

move to new abseil builds

0e11a1f

switch to shared builds / cmake+ninja

2108249

add initial set of patches

15573ef

developed in conda-forge#26

add libprotobuf dependency to sentencepiece

3c0d18d

move away from setup.py

98dc594

build static lib on windows

051898e

using our protobuf also for cross-compilation

f5098f2

point to our libs on win in setup.py

9fa12fb

use -DCMAKE_BUILD_TYPE=Release

cfaac15

don't force-replace /MD with /MT

ab6eedd

set USE_DLL env vars for python bindings on windows

a1c283f

also build pkg-config on windows; add tests

3138b8e

bump build number

a3cb6c6

MNT: Re-rendered with conda-build 3.23.3, conda-smithy 3.22.1, and co…

71838be

…nda-forge-pinning 2022.12.11.21.21.05

h-vetinari force-pushed the lib branch from 0c5db76 to 71838be Compare December 12, 2022 06:17

h-vetinari changed the title ~~WIP: Build lib separately (shared on unix; stay static on win)~~ Build lib separately (shared on unix; stay static on win) Dec 12, 2022

h-vetinari commented Dec 12, 2022

View reviewed changes

xhochy reviewed Dec 12, 2022

View reviewed changes

h-vetinari added 2 commits December 14, 2022 21:36

remove (hopefully) unnecessary environment variables

09564a7

add outputs sentencepiece-{spm,python}, turn sentencepiece into metap…

1b2f846

…ackage

h-vetinari force-pushed the lib branch from 909d5e0 to 1b2f846 Compare December 14, 2022 11:02

conda-forge deleted a comment from conda-forge-linter Dec 14, 2022

carterbox approved these changes Dec 16, 2022

View reviewed changes

recipe/build-lib.sh Show resolved Hide resolved

MNT: Re-rendered with conda-build 3.23.3, conda-smithy 3.22.1, and co…

4b266e9

…nda-forge-pinning 2022.12.15.19.46.12

carterbox suggested changes Dec 16, 2022

View reviewed changes

recipe/meta.yaml Outdated Show resolved Hide resolved

add run-export also to metapackage

83d4852

Co-authored-by: Daniel Ching <carterbox@users.noreply.github.com>

carterbox approved these changes Dec 16, 2022

View reviewed changes

setu4993 approved these changes Dec 17, 2022

View reviewed changes

h-vetinari merged commit 7ad3705 into conda-forge:main Dec 17, 2022

h-vetinari deleted the lib branch December 17, 2022 20:43

h-vetinari mentioned this pull request Dec 26, 2022

sentencepiece 0.1.97 #34

Merged

h-vetinari mentioned this pull request Jan 20, 2023

make libsentencepiece run-depend on libabseil; add full compilation test #36

Merged

This was referenced Jun 6, 2023

Patches carried by conda-forge for packaging sentencepiece google/sentencepiece#876

Open

[Feature request] Add CLI support for Windows and macOS #21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build lib separately (shared on unix; stay static on win) #33

Build lib separately (shared on unix; stay static on win) #33

h-vetinari commented Dec 10, 2022 •

edited

Loading

conda-forge-linter commented Dec 10, 2022

h-vetinari left a comment

h-vetinari Dec 12, 2022

h-vetinari Dec 12, 2022

xhochy Dec 12, 2022

h-vetinari Dec 12, 2022

xhochy commented Dec 12, 2022

hmaarrfk commented Dec 12, 2022

carterbox commented Dec 12, 2022

h-vetinari commented Dec 12, 2022

carterbox commented Dec 12, 2022

h-vetinari commented Dec 12, 2022

h-vetinari commented Dec 14, 2022

carterbox commented Dec 16, 2022

carterbox commented Dec 16, 2022

setu4993 left a comment

		@@ -0,0 +1,884 @@
		From 9f225f6f83ccea1909974ee95da9b724d017dad3 Mon Sep 17 00:00:00 2001

Build lib separately (shared on unix; stay static on win) #33

Build lib separately (shared on unix; stay static on win) #33

Conversation

h-vetinari commented Dec 10, 2022 • edited Loading

conda-forge-linter commented Dec 10, 2022

h-vetinari left a comment

Choose a reason for hiding this comment

h-vetinari Dec 12, 2022

Choose a reason for hiding this comment

h-vetinari Dec 12, 2022

Choose a reason for hiding this comment

xhochy Dec 12, 2022

Choose a reason for hiding this comment

h-vetinari Dec 12, 2022

Choose a reason for hiding this comment

xhochy commented Dec 12, 2022

hmaarrfk commented Dec 12, 2022

carterbox commented Dec 12, 2022

h-vetinari commented Dec 12, 2022

carterbox commented Dec 12, 2022

h-vetinari commented Dec 12, 2022

h-vetinari commented Dec 14, 2022

carterbox commented Dec 16, 2022

carterbox commented Dec 16, 2022

setu4993 left a comment

Choose a reason for hiding this comment

h-vetinari commented Dec 10, 2022 •

edited

Loading