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

(ci) fix integrationtest vectorsearch #981

Merged
merged 8 commits into from
Nov 22, 2024

Conversation

thomasht86
Copy link
Collaborator

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

This integration test has been unstable and slooow. 🐌
Some updates to improve stability and speed.

  • reuse common environment and run workflows in parallel
  • comment out prod deployment (not only skip)
  • switch to Python 3.10

PR run here: https://github.com/vespa-engine/pyvespa/actions/runs/11970588855

@thomasht86 thomasht86 requested a review from esolitos November 22, 2024 10:33
Copy link
Contributor

@esolitos esolitos left a comment

Choose a reason for hiding this comment

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

I'm not sure why you wouldn't reuse the cache and instead upload it as artifact?

What I would typically in this situation is one of those:

a. Have a "build" job which builds the application and stores an actions/upload-artifact. The dependant jobs only get the built artifact and don't run any install steps.

b. Don't have a build job and simply use actions/cache with restore-keys that can fallback on a "older version" of the caches perfect match fails.

You seem to have mixed up caches and artifact and I don't see the benefit in this approach, unless I am missing something, that is. 😅


- name: Cache dependencies
id: cache-python
uses: actions/cache@v3
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uses: actions/cache@v3
uses: actions/cache@v4

Comment on lines 41 to 45
- name: Upload Python environment
uses: actions/upload-artifact@v3
with:
name: python-environment
path: ~/.cache/pip
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why you would use this instead of simply rely on actions/cache for restoring as well?

Comment on lines 58 to 62
- name: Download Python environment
uses: actions/download-artifact@v3
with:
name: python-environment
path: ~/.cache/pip
Copy link
Contributor

Choose a reason for hiding this comment

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

Use actions/cache + restore-key instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, outdated version:

Suggested change
- name: Download Python environment
uses: actions/download-artifact@v3
with:
name: python-environment
path: ~/.cache/pip
- name: Download Python environment
uses: actions/download-artifact@v4
with:
name: python-environment
path: ~/.cache/pip

@thomasht86
Copy link
Collaborator Author

Thanks! I will have another look at the cache-action to see if we can skip the artifact upload/download!

@thomasht86 thomasht86 force-pushed the thomasht86/fix-integrationtest-vectorsearch branch from 9b081bc to d9fde8d Compare November 22, 2024 13:39
@thomasht86
Copy link
Collaborator Author

Ok, so it is 2 different things we ideally would want 2 cache.

  1. Pip packages directory. This is now handled automatically by setup-python-action.
  2. It would also be nice to cache the generated environment after installing pyvespa from source. This would probably require a more advanced caching, but not so much time to save on this, so leaving that for now.

@thomasht86 thomasht86 requested a review from esolitos November 22, 2024 14:23
esolitos
esolitos previously approved these changes Nov 22, 2024
Copy link
Contributor

@esolitos esolitos left a comment

Choose a reason for hiding this comment

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

👍🏼

.github/workflows/integration-cloud.yml Outdated Show resolved Hide resolved
Amazing! Learned a new trick today as well :D

Co-authored-by: Marlon (Esolitos) Saglia <[email protected]>
@thomasht86 thomasht86 merged commit daa1679 into master Nov 22, 2024
42 of 49 checks passed
@thomasht86 thomasht86 deleted the thomasht86/fix-integrationtest-vectorsearch branch November 22, 2024 14:43
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.

2 participants