Polber commented on code in PR #29958:
URL: https://github.com/apache/beam/pull/29958#discussion_r1446514564
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -730,36 +734,50 @@ def _path(cls, base_python, packages):
def _create_venv_from_scratch(cls, base_python, packages):
venv = cls._path(base_python, packages)
if not os.path.exists(venv):
- subprocess.run([base_python, '-m', 'venv', venv], check=True)
- venv_python = os.path.join(venv, 'bin', 'python')
- subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
- subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
- check=True)
- with open(venv + '-requirements.txt', 'w') as fout:
- fout.write('\n'.join(packages))
+ try:
+ subprocess.run([base_python, '-m', 'venv', venv], check=True)
+ venv_python = os.path.join(venv, 'bin', 'python')
+ subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
+ subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
+ check=True)
Review Comment:
I have a branch with most of these changes as well (just hadn't gotten
around to cleaning it and opening a PR yet), but one change I noticed I had
that isn't here is for the `pip install` commands. For some reason subprocess
does not cause the execution to fail when the command does not pass (despite
`check=True`), but I noticed that `python -m pip install ...` was not actually
running, so dependencies were not getting installed. This change worked for me:
```suggestion
venv_python = os.path.join(venv, 'bin', 'python')
venv_pip = os.path.join(venv, 'bin', 'pip')
subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
subprocess.run([venv_pip, 'install'] + packages,
check=True)
```
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -730,36 +734,50 @@ def _path(cls, base_python, packages):
def _create_venv_from_scratch(cls, base_python, packages):
venv = cls._path(base_python, packages)
if not os.path.exists(venv):
- subprocess.run([base_python, '-m', 'venv', venv], check=True)
- venv_python = os.path.join(venv, 'bin', 'python')
- subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
- subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
- check=True)
- with open(venv + '-requirements.txt', 'w') as fout:
- fout.write('\n'.join(packages))
+ try:
+ subprocess.run([base_python, '-m', 'venv', venv], check=True)
+ venv_python = os.path.join(venv, 'bin', 'python')
+ subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
+ subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
+ check=True)
+ with open(venv + '-requirements.txt', 'w') as fout:
+ fout.write('\n'.join(packages))
+ except: # pylint: disable=bare-except
+ if os.path.exists(venv):
+ shutil.rmtree(venv, ignore_errors=True)
+ raise
return venv
@classmethod
def _create_venv_from_clone(cls, base_python, packages):
venv = cls._path(base_python, packages)
if not os.path.exists(venv):
- clonable_venv = cls._create_venv_to_clone(base_python)
- clonable_python = os.path.join(clonable_venv, 'bin', 'python')
- subprocess.run(
- [clonable_python, '-m', 'clonevirtualenv', clonable_venv, venv],
- check=True)
- venv_binary = os.path.join(venv, 'bin', 'python')
- subprocess.run([venv_binary, '-m', 'pip', 'install'] + packages,
- check=True)
- with open(venv + '-requirements.txt', 'w') as fout:
- fout.write('\n'.join(packages))
+ try:
+ clonable_venv = cls._create_venv_to_clone(base_python)
+ clonable_python = os.path.join(clonable_venv, 'bin', 'python')
+ subprocess.run(
+ [clonable_python, '-m', 'clonevirtualenv', clonable_venv, venv],
+ check=True)
+ venv_binary = os.path.join(venv, 'bin', 'python')
+ subprocess.run([venv_binary, '-m', 'pip', 'install'] + packages,
+ check=True)
Review Comment:
Same here
```suggestion
venv_pip = os.path.join(venv, 'bin', 'pip')
subprocess.run([venv_pip, 'install'] + packages,
check=True)
```
##########
sdks/python/apache_beam/yaml/yaml_provider.py:
##########
@@ -730,36 +734,50 @@ def _path(cls, base_python, packages):
def _create_venv_from_scratch(cls, base_python, packages):
venv = cls._path(base_python, packages)
if not os.path.exists(venv):
- subprocess.run([base_python, '-m', 'venv', venv], check=True)
- venv_python = os.path.join(venv, 'bin', 'python')
- subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
- subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
- check=True)
- with open(venv + '-requirements.txt', 'w') as fout:
- fout.write('\n'.join(packages))
+ try:
+ subprocess.run([base_python, '-m', 'venv', venv], check=True)
+ venv_python = os.path.join(venv, 'bin', 'python')
+ subprocess.run([venv_python, '-m', 'ensurepip'], check=True)
+ subprocess.run([venv_python, '-m', 'pip', 'install'] + packages,
+ check=True)
+ with open(venv + '-requirements.txt', 'w') as fout:
+ fout.write('\n'.join(packages))
+ except: # pylint: disable=bare-except
+ if os.path.exists(venv):
+ shutil.rmtree(venv, ignore_errors=True)
+ raise
return venv
@classmethod
def _create_venv_from_clone(cls, base_python, packages):
venv = cls._path(base_python, packages)
if not os.path.exists(venv):
- clonable_venv = cls._create_venv_to_clone(base_python)
- clonable_python = os.path.join(clonable_venv, 'bin', 'python')
- subprocess.run(
- [clonable_python, '-m', 'clonevirtualenv', clonable_venv, venv],
- check=True)
- venv_binary = os.path.join(venv, 'bin', 'python')
- subprocess.run([venv_binary, '-m', 'pip', 'install'] + packages,
- check=True)
- with open(venv + '-requirements.txt', 'w') as fout:
- fout.write('\n'.join(packages))
+ try:
+ clonable_venv = cls._create_venv_to_clone(base_python)
+ clonable_python = os.path.join(clonable_venv, 'bin', 'python')
+ subprocess.run(
+ [clonable_python, '-m', 'clonevirtualenv', clonable_venv, venv],
+ check=True)
+ venv_binary = os.path.join(venv, 'bin', 'python')
+ subprocess.run([venv_binary, '-m', 'pip', 'install'] + packages,
+ check=True)
+ with open(venv + '-requirements.txt', 'w') as fout:
+ fout.write('\n'.join(packages))
+ except: # pylint: disable=bare-except
+ if os.path.exists(venv):
+ shutil.rmtree(venv, ignore_errors=True)
+ raise
return venv
@classmethod
def _create_venv_to_clone(cls, base_python):
+ if '.dev' in beam_version:
+ base_venv = os.path.dirname(os.path.dirname(base_python))
+ print('Cloning dev environment from', base_venv)
return cls._create_venv_from_scratch(
- base_python, [
- 'apache_beam[dataframe,gcp,test]==' + beam_version,
+ base_python,
+ [
+ 'apache_beam[dataframe,gcp,test,yaml]==' + beam_version,
Review Comment:
I found that this didn't work with `.dev` versions (again the error is not
surfaced), so I tried using:
```suggestion
'apache_beam[dataframe,gcp,test]' + (f'=={beam_version}' if
'.dev' not in beam_version else ''),
```
But this was problematic because the runner would see different versions
(i.e. 2.53.0 vs. 2.54.0.dev), so I don't think this is a viable solution, but
if I remember correctly, the docker container will throw an error about not
being able to find `apache_beam==2.54.0.dev` once the `pip install` is fixed
via my other comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
