With this PR:
- All lint and test jobs use the exact same Python + Poetry installation
approach, instead of lints doing it one way and tests doing it another
way.
- The Poetry installation itself is cached, which saves ~15s per run.
- We no longer pass shell commands as workflow arguments to a workflow
that just runs them in a shell. This makes our actions more resilient to
shell code injection.
If y'all like this approach, I can modify the scheduled tests workflow
and the release workflow to use this too.
If another push to the same PR or branch happens while its CI is still
running, cancel the earlier run in favor of the next run.
There's no point in testing an outdated version of the code. GitHub only
allows a limited number of job runners to be active at the same time, so
it's better to cancel pointless jobs early so that more useful jobs can
run sooner.
It's possible that langchain-experimental works fine with the latest
*published* langchain, but is broken with the langchain on `master`.
Unfortunately, you can see this is currently the case — this is why this
PR also includes a minor fix for the `langchain` package itself.
We want to catch situations like that *before* releasing a new
langchain, hence this test.
The current timeouts are too long, and mean that if the GitHub cache
decides to act up, jobs get bogged down for 15min at a time. This has
happened 2-3 times already this week -- a tiny fraction of our total
workflows but really annoying when it happens to you. We can do better.
Installing deps on cache miss takes about ~4min, so it's not worth
waiting more than 4min for the deps cache. The black and mypy caches
save 1 and 2min, respectively, so wait only up to that long to download
them.
The previous approach was relying on `_test.yml` taking an input
parameter, and then doing almost completely orthogonal things for each
parameter value. I've separated out each of those test situations as its
own job or workflow file, which eliminated all the special-casing and,
in my opinion, improved maintainability by making it much more obvious
what code runs when.
Trusted Publishing is the current best practice for publishing Python
packages. Rather than long-lived secret keys, it uses OpenID Connect
(OIDC) to allow our GitHub runner to directly authenticate itself to
PyPI and get a short-lived publishing token. This locks down publishing
quite a bit:
- There's no long-lived publish key to steal anymore.
- Publishing is *only* allowed via the *specifically designated* GitHub
workflow in the designated repo.
It also is operationally easier: no keys means there's nothing that
needs to be periodically rotated, nothing to worry about leaking, and
nobody can accidentally publish a release from their laptop because they
happened to have PyPI keys set up.
After this gets merged, we'll need to configure PyPI to start expecting
trusted publishing. It's only a few clicks and should only take a
minute; instructions are here:
https://docs.pypi.org/trusted-publishers/adding-a-publisher/
More info:
- https://blog.pypi.org/posts/2023-04-20-introducing-trusted-publishers/
- https://github.com/pypa/gh-action-pypi-publish
This is safer than the prior approach, since it's safe by default: the
release workflows never get triggered for non-merged PRs, so there's no
possibility of a buggy conditional accidentally letting a workflow
proceed when it shouldn't have.
The only loss is that publishing no longer requires a `release` label on
the merged PR that bumps the version. We can add a separate CI step that
enforces that part as a condition for merging into `master`, if
desirable.
I have discovered a bug located within `.github/workflows/_release.yml`
which is the primary cause of continuous integration (CI) errors. The
problem can be solved; therefore, I have constructed a PR to address the
issue.
## The Issue
Access the following link to view the exact errors: [Langhain Release
Workflow](https://github.com/langchain-ai/langchain/actions/workflows/langchain_release.yml)
The instances of these errors take place for **each PR** that updates
`pyproject.toml`, excluding those specifically associated with bumping
PRs.
See below for the specific error message:
```
Error: Error 422: Validation Failed: {"resource":"Release","code":"already_exists","field":"tag_name"}
```
An image of the error can be viewed here:
The `_release.yml` document contains the following if-condition:
```yaml
if: |
${{ github.event.pull_request.merged == true }}
&& ${{ contains(github.event.pull_request.labels.*.name, 'release') }}
```
## The Root Cause
The above job constantly runs as the `if-condition` is always identified
as `true`.
## The Logic
The `if-condition` can be defined as `if: ${{ b1 }} && ${{ b2 }}`, where
`b1` and `b2` are boolean values. However, in terms of condition
evaluation with GitHub Actions, `${{ false }}` is identified as a string
value, thereby rendering it as truthy as per the [official
documentation](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idif).
I have run some tests regarding this behavior within my forked
repository. You can consult my [debug
PR](https://github.com/zawakin/langchain/pull/1) for reference.
Here is the result of the tests:
|If-Condition|Outcome|
|:--:|:--:|
|`if: true && ${{ false }}`|Execution|
|`if: ${{ false }}` |Skipped|
|`if: true && false` |Skipped|
|`if: false`|Skipped|
|`if: ${{ true && false }}` |Skipped|
In view of the first and second results, we can infer that `${{ false
}}` can only be interpreted as `true` for conditions composed of some
expressions.
It is consistent that the condition of `if: ${{ inputs.working-directory
== 'libs/langchain' }}` works.
It is surprised to be skipped for the second case but it seems the spec
of GitHub Actions 😓
Anyway, the PR would fix these errors, I believe 👍
Could you review this? @hwchase17 or @shoelsch , who is the author of
[PR](https://github.com/langchain-ai/langchain/pull/360).
Ternary operators in GitHub Actions syntax are pretty ugly and hard to
read: `inputs.working-directory == '' && '.' ||
inputs.working-directory` means "if the condition is true, use `'.'` and
otherwise use the expression after the `||`".
This PR performs the ternary as few times as possible, assigning its
outcome to an env var we can then reuse as needed.
Only lint on the min and max supported Python versions.
It's extremely unlikely that there's a lint issue on any version in
between that doesn't show up on the min or max versions.
GitHub rate-limits how many jobs can be running at any one time.
Starting new jobs is also relatively slow, so linting on fewer versions
makes CI faster.
Using `poetry add` to install `pydantic@2.1` was also causing poetry to
change its lockfile. This prevented dependency caching from working:
- When attempting to restore a cache, it would hash the lockfile in git
and use it as part of the cache key. Say this is a cache miss.
- Then, it would attempt to save the cache -- but the lockfile will have
changed, so the cache key would be *different* than the key in the
lookup. So the cache save would succeed, but to a key that cannot be
looked up in the next run -- meaning we never get a cache hit.
In addition to busting the cache, the lockfile update itself is also
non-trivially long, over 30s:
This PR fixes the problems by using `pip` to perform the installation,
avoiding the lockfile change.
* PR updates test.yml to test with both pydantic versions
* Code should be refactored to make it easier to do testing in matrix
format w/ packages
* Added steps to assert that pydantic version in the environment is as
expected
Probably the most boring PR to review ;)
Individual commits might be easier to digest
---------
Co-authored-by: Bagatur <baskaryan@gmail.com>
Co-authored-by: Bagatur <22008038+baskaryan@users.noreply.github.com>
# Only run linkchecker on direct changes to docs
This is a stop-gap that will speed up PRs.
Some broken links can slip through if they're embedded in doc-strings
inside the codebase.
But we'll still be running the linkchecker on master.
# Check poetry lock file on CI
This PR checks that the lock file is up to date using poetry lock
--check.
As part of this PR, a new lock file was generated.
# Refactor the test workflow
This PR refactors the tests to run using a single test workflow. This
makes it easier to relaunch failing tests and see in the UI which test
failed since the jobs are grouped together.
## Before submitting
## Who can review?
# Add action to test with all dependencies installed
PR adds a custom action for setting up poetry that allows specifying a
cache key:
https://github.com/actions/setup-python/issues/505#issuecomment-1273013236
This makes it possible to run 2 types of unit tests:
(1) unit tests with only core dependencies
(2) unit tests with extended dependencies (e.g., those that rely on an
optional pdf parsing library)
As part of this PR, we're moving some pdf parsing tests into the
unit-tests section and making sure that these unit tests get executed
when running with extended dependencies.
Adds release workflow that (1) creates a GitHub release and (2)
publishes built artifacts to PyPI
**Release Workflow**
1. Checkout `master` locally and cut a new branch
1. Run `poetry version <rule>` to version bump (e.g., `poetry version
patch`)
1. Commit changes and push to remote branch
1. Ensure all quality check workflows pass
1. Explicitly tag PR with `release` label
1. Merge to mainline
At this point, a release workflow should be triggered because:
* The PR is closed, targeting `master`, and merged
* `pyproject.toml` has been detected as modified
* The PR had a `release` label
The workflow will then proceed to build the artifacts, create a GitHub
release with release notes and uploaded artifacts, and publish to PyPI.
Example Workflow run:
https://github.com/shoelsch/langchain/actions/runs/3711037455/jobs/6291076898
Example Releases: https://github.com/shoelsch/langchain/releases
--
Note, this workflow is looking for the `PYPI_API_TOKEN` secret, so that
will need to be uploaded to the repository secrets. I tested uploading
as far as hitting a permissions issue due to project ownership in Test
PyPI.
