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
chore(ci): fix flakiness, misc improvements #7605
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if we want to remove the venv cache wholesale vs just commenting out
id: cache-venv | ||
with: | ||
path: ./smoke-test/venv/ | ||
key: smoke-venv-${{ runner.os }}-${{ steps.packages_checksum.outputs.packages_checksum }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we comment this out instead? Do you actually know what the issue was here or just disabling because of issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no difference in run time. See master
and this branch's runs. No time difference is there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird - that probably means the caching wasn't even working?
path: ./metadata-ingestion/venv/ | ||
key: ${{ runner.os }}-venv-${{ steps.packages_checksum.outputs.packages_checksum }}-${{ matrix.python-version }} | ||
- name: Install package | ||
run: ./gradlew :metadata-ingestion:installPackageOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this just for debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Wanted to see how much time installing the package takes separately to the tests.
@@ -12,7 +12,8 @@ if (!project.hasProperty("extra_pip_requirements")) { | |||
} | |||
|
|||
task checkPythonVersion(type: Exec) { | |||
commandLine python_executable, '-c', 'import sys; assert sys.version_info >= (3, 7)' | |||
commandLine python_executable, '-c', | |||
'import sys; assert (3, 11) > sys.version_info >= (3, 7), f"Python version {sys.version_info[:2]} not allowed"' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change message to "Is too old"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? If someone is using 3.11 it won't be too old but too new.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i missed the 3.11 part - then this is fine
Also it seems strange that we've seen issues in smoke test but not with metadata ingestion yet. Any ideas why that could be? |
venv
containspython
. Caching wholevenv
instead of justpip
cache means you are backing up python which is problematic and is probably causing python not being found randomly in the smoke tests. If it is causing issues with smoke tests it would also cause issues withmetadata-ingestion
so removing that too.e.g. https://github.com/datahub-project/datahub/actions/runs/4433850334/jobs/7779664856 where log says
Python not being found is really odd as we have a setup python step. It is probably the
venv
cache after that which is messing things somehow.Checklist