Skip to content

Conversation

@pabloem
Copy link
Member

@pabloem pabloem commented Apr 16, 2018

No description provided.

@pabloem
Copy link
Member Author

pabloem commented Apr 16, 2018

@pabloem pabloem force-pushed the fix-python-precommit branch 3 times, most recently from 6f59664 to 6430976 Compare April 17, 2018 17:16
@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

Other latest Python PreCommits are running in beam3 and show failures in setupVirtualenv, which seems to indicate environment setup issues.
@aaltay do you think adding this restriction is reasonable?
beam3 - https://builds.apache.org/job/beam_PreCommit_Python_GradleBuild/534/console

@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

I'm also adding a check for existence of a directory before removal in gen_protos.

@pabloem pabloem force-pushed the fix-python-precommit branch from 6430976 to 4146b7d Compare April 17, 2018 17:32
Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

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

Approving this to enable tests. I do not think I understand why nodes become bad or recover later. Is it possible that our tests are leaving things in an invalid state?

finally:
sys.stderr.flush()
shutil.rmtree(build_path)
if os.path.exists(build_path):
Copy link
Member

Choose a reason for hiding this comment

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

I thought we decided to use
shutil.rmtree(build_path, ignore_errors=True instead of a conditional check (https://docs.python.org/2/library/shutil.html#shutil.rmtree)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah I'm sorry. Good memory. We had not merged that fix, but I'll make the change now.

@pabloem pabloem force-pushed the fix-python-precommit branch from 4146b7d to acd6399 Compare April 17, 2018 18:28
@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

I've checked with Jason. I asked why Python tests pass/fail on some machines:

The reasons for that are as many as the stars in the sky
Different package versions, transient network issues, permanent network issues, Jenkins instability, etc.
For Python, historically there were some machines with up-to-date pip and virtualenv and some without, and we restricted execution of Python tests to the ones with

It doesn't seem that old Maven-based precommit had any sort of restrictions on hosts, so I'm not sure what's going on in this case: https://github.com/apache/beam/blob/a25157bc6a9203362f6dd23acc8b7e1f30daad8e/.test-infra/jenkins/job_beam_PreCommit_Python_MavenInstall.groovy

Somehow it must be that the Gradle build and the Maven build had different assumptions?

@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

Run Seed Job

@pabloem
Copy link
Member Author

pabloem commented Apr 17, 2018

Retest this please

@aaltay aaltay merged commit 4cf6c54 into apache:master Apr 17, 2018
@robertwb
Copy link
Contributor

The maven-based precommit didn't use virtualenv (it modified and used the users install of Python). This should be more hermetic, assuming a recent virtualenv and pip.

@pabloem pabloem deleted the fix-python-precommit branch April 17, 2018 21:57
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
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.

4 participants