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

add makefile to requirements #1295

Merged
merged 18 commits into from
Nov 2, 2023
Merged

add makefile to requirements #1295

merged 18 commits into from
Nov 2, 2023

Conversation

badGarnet
Copy link
Collaborator

@badGarnet badGarnet commented Sep 4, 2023

This PR resolves #1294 by adding a Makefile to compile requirements. This makefile respects the dependencies between file and will compile them in order. E.g., extra-*.txt will be compiled after base.txt is updated.

Test locally by simply running make pip-compile or cd requirements && make clean && make all

@cragwolfe
Copy link
Contributor

why this is better than the existing solution? in the off chance that the existing make pip-compile results in inconsistent dependencies, this would get caught by https://github.com/Unstructured-IO/unstructured/blob/b710baf/.github/workflows/ci.yml#L70 .

but as long as pins are being kept in constraints.in, this really shouldn't happen in the first place.

@badGarnet
Copy link
Collaborator Author

why this is better than the existing solution? in the off chance that the existing make pip-compile results in inconsistent dependencies, this would get caught by https://github.com/Unstructured-IO/unstructured/blob/b710baf/.github/workflows/ci.yml#L70 .

but as long as pins are being kept in constraints.in, this really shouldn't happen in the first place.

The current method relies on base.txt is compiled first because of alphabetical order. The other extras that depend on base.txt are able to get updated base.txt because of naming. Additionally if we have more layered dependencies like dev.txt we may not be able to rely on the file names.

Makefile is designed to handle compilation dependency so I would argue that is the safest way to ensure pip-compile runs correctly. Now I do concede that with current requirements this may seem a bit over engineering.

@badGarnet
Copy link
Collaborator Author

probably not needed for now

@badGarnet badGarnet closed this Sep 7, 2023
@badGarnet badGarnet reopened this Oct 5, 2023
@badGarnet
Copy link
Collaborator Author

reopen this PR because recently we have encountered a case where dev.in was compiled before test.txt was compiled fresh, and results in incompatible versions of dependencies that fails ci: https://github.com/Unstructured-IO/unstructured/actions/runs/6421527738/job/17436375115; https://github.com/Unstructured-IO/unstructured/actions/runs/6421723279/job/17436965312

@badGarnet badGarnet requested review from rbiseck3 and qued October 5, 2023 17:38
@rbiseck3
Copy link
Contributor

rbiseck3 commented Oct 5, 2023

Got this:

❯ make pip-compile
scripts/pip-compile.sh: line 11: MAKE[1]:: command not found
cp: requirements/build.txt: No such file or directory
make: *** [pip-compile] Error 1

@@ -8,5 +8,5 @@ if ! python -c "import sys; assert sys.version_info.major == $major and sys.vers
exit 1
fi

cd ./requirements && $(MAKE) *.txt
cd ./requirements && make *.txt && cd ..
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use pushd/popd to navigate to/from the requirements dir. Has some pleasantries: Navigating with popd

@badGarnet badGarnet marked this pull request as ready for review October 26, 2023 16:06
@badGarnet badGarnet requested a review from rbiseck3 October 26, 2023 16:06
@qued
Copy link
Contributor

qued commented Oct 26, 2023

It looks like everything in the ingest subdirectory of requirements is being skipped.

@qued
Copy link
Contributor

qued commented Oct 26, 2023

Also there used to be logic to copy the build.txt file in requirements/ into the docs/ folder overwriting docs/requirements.txt. In other words, the file build.txt is really the requirements file for sphinx builds. I think that logic is missing from what we are currently doing as well.

@badGarnet
Copy link
Collaborator Author

Also there used to be logic to copy the build.txt file in requirements/ into the docs/ folder overwriting docs/requirements.txt. In other words, the file build.txt is really the requirements file for sphinx builds. I think that logic is missing from what we are currently doing as well.

updated makefile to work on subdirectories and added back the copy line

@badGarnet badGarnet temporarily deployed to ci October 30, 2023 21:05 — with GitHub Actions Inactive
@badGarnet badGarnet temporarily deployed to ci October 30, 2023 21:07 — with GitHub Actions Inactive
@badGarnet badGarnet temporarily deployed to ci October 30, 2023 21:07 — with GitHub Actions Inactive
@badGarnet badGarnet temporarily deployed to ci October 30, 2023 21:07 — with GitHub Actions Inactive
@badGarnet badGarnet temporarily deployed to ci October 30, 2023 21:07 — with GitHub Actions Inactive
@badGarnet badGarnet added this pull request to the merge queue Oct 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 30, 2023
@qued qued temporarily deployed to ci November 1, 2023 14:16 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 1, 2023 14:30 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 1, 2023 14:30 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 1, 2023 14:30 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 1, 2023 14:30 — with GitHub Actions Inactive
@qued qued added this pull request to the merge queue Nov 1, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2023
@badGarnet badGarnet added this pull request to the merge queue Nov 1, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2023
@qued qued temporarily deployed to ci November 2, 2023 15:17 — with GitHub Actions Inactive
@qued qued merged commit 6926568 into main Nov 2, 2023
24 checks passed
@qued qued deleted the yao/add-make-pip-compile branch November 2, 2023 15:17
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
@qued qued temporarily deployed to ci November 2, 2023 15:21 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat/pip-compile-make-command
4 participants