Skip to content
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
9 changes: 9 additions & 0 deletions .github/renovate.json5
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"$schema": "https://docs.renovatebot.com/renovate-schema.json",
"extends": [
"config:recommended",
"schedule:weekly"
],
"timezone": "America/Los_Angeles",
"includePaths": [".github/**"]
}

Choose a reason for hiding this comment

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

Sorry I'm not that familiar with renovate configuration -- what effect does this have? I assumed it mainly defined where to look for things to upgrade. Are there dependencies in this repo it'll manage?

Choose a reason for hiding this comment

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

If renovate will target this repo, does it need to be added here: https://github.com/Everlaw/renovate-bot-config/blob/main/.renovate/config.js ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sorry I'm not that familiar with renovate configuration -- what effect does this have? I assumed it mainly defined where to look for things to upgrade. Are there dependencies in this repo it'll manage?

In this case it'll probably only update

- uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29

If renovate will target this repo, does it need to be added here: https://github.com/Everlaw/renovate-bot-config/blob/main/.renovate/config.js ?

Yep! https://github.com/Everlaw/renovate-bot-config/pull/120

36 changes: 36 additions & 0 deletions .github/workflows/build_wheel.yml
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For reference, this implementation is mostly copy-pasted from our existing CI for Everlaw/libpff: https://github.com/Everlaw/libpff/blob/master/.github/workflows/build_wheel.yml

Choose a reason for hiding this comment

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

I'm sure ya'll considered this when implementing libpff, but I'm curious how we decided what to put in the servers repo vs what we put in the host repo?
For example, the - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 line seems like it would be easier to find/replace if it were in servers.

Choose a reason for hiding this comment

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

Same with the python and pip versions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For example, the - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 line seems like it would be easier to find/replace if it were in servers.

This will get updated by Renovate like Everlaw/libpff#10... though I'm realizing we probably need to configure the bot to automatically assign reviewers 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm realizing we probably need to configure the bot to automatically assign reviewers 😆

Everlaw/libpff#15

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, the "checkout" thing needs to be in this repo, because that's how we actually perform the git checkout https://github.com/actions/checkout?tab=readme-ov-file#checkout-v4.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same with the python

This one actually needs to be independent of what's in https://github.com/Everlaw/servers. If we were going to upgrade to, say, python3.13, we'd add a new item to

so that we end up pushing a cp313 wheel to S3. Then, https://github.com/Everlaw/servers would be able to download and install it.

and pip versions

These are just matching the original (pre-fork) constraints, so I don't expect them to ever change. In any case, the constraints for this library should be completely independent from the constraints for https://github.com/Everlaw/servers.

Choose a reason for hiding this comment

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

In any case, the constraints for this library should be completely independent from the constraints for

Would we not want to change this python version along with our global python version whenever we update?

Choose a reason for hiding this comment

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

At any rate, sounds like you've got a much better understanding of this as an ecosystem than I do! Just looking out for a situation where, say, someone is updating our python version and forgets to do so here then has to get all the way through some CI build before it reports a certain artifact not being found, have to track down this repo, change it here as well etc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

someone is updating our python version and forgets to do so here then has to get all the way through some CI build before it reports a certain artifact not being found, have to track down this repo, change it here as well etc.

Haha yes, that's likely how it will happen.

We just need to be able to support deploying multiple artifacts (one for each downstream Python version) at the same time so that the update PR into this repo can land independent of the "main" PR into https://github.com/Everlaw/servers.

Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
name: build_wheel
on: [push]
permissions: read-all
jobs:
build_wheel:
strategy:
matrix:
python-version:
- 3.12.6
runs-on: [self-hosted, fasttext]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The fasttext runner is being added in https://github.com/Everlaw/servers/pull/39904.

steps:
- uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29
- name: Install Python
run: |
uv python install ${{ matrix.python-version }}
- name: Create virtual environment
run: |
rm -rf venv
uv venv --python ${{ matrix.python-version }} venv
echo "$(pwd)/venv/bin" >> $GITHUB_PATH
- name: Build wheel
run: |
rm -rf dist
uv build --out-dir dist
- name: Check wheel
run: |
# These requirements were lifted from `install_requires` in `setup.py`.
uv pip install \
'pybind11>=2.2' \
'setuptools>=0.7.0' \
'numpy'
uv pip install --no-index --find-links dist fasttext
Comment on lines +27 to +32
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The goal here is to install the fasttext wheel we just built. We use --no-index --find-links dist to ensure we don't accidentally pull anything unexpected from PyPI.

python -c 'import fasttext'
- name: Upload wheel
run: |
./upload-wheel.sh dist/fasttext*.whl
2 changes: 2 additions & 0 deletions CODEOWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
# https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners
* @Everlaw/developer-experience-squad
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
[metadata]
description-file = README.md
description_file = README.md
3 changes: 2 additions & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
import platform
import io

__version__ = "0.9.2"
__version__ = "0.9.4.rc0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Once we're satisfied with testing https://github.com/Everlaw/servers/pull/39905, we should remove the .rc0 suffix.

FASTTEXT_SRC = "src"

# Based on https://github.com/pybind/python_example
Expand Down Expand Up @@ -194,6 +194,7 @@ def _get_readme():
"Operating System :: Unix",
"Operating System :: MacOS",
],
# These requirements are duplicated in `.github/workflows/build_wheel.yml`.
install_requires=["pybind11>=2.2", "setuptools >= 0.7.0", "numpy"],
cmdclass={"build_ext": BuildExt},
packages=[
Expand Down
14 changes: 14 additions & 0 deletions upload-wheel.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#!/bin/bash

set -euo pipefail

wheel="$1"
url="s3://everlaw-build-artifacts/fasttext/$(basename "$wheel")"

if aws s3 ls "$url" &>/dev/null; then
>&2 echo "$url already exists!"
exit 0
fi

aws s3 cp --only-show-errors "$wheel" "$url"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The runner has permission to do this because of https://github.com/Everlaw/servers/pull/39903.

>&2 echo "Uploaded $url"
Loading