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

Check versions used to create the venv and auto-wipe #3432

Merged
merged 12 commits into from Dec 28, 2017

Conversation

Projects
None yet
5 participants
@humitos
Member

humitos commented Dec 21, 2017

Initial drawing of ideas on how to implement #3190

@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 21, 2017

Looks like a good start!

@humitos

This comment has been minimized.

Member

humitos commented Dec 21, 2017

This is getting better, I think. Some notes on this:

  • old venv without environment.json will always return is_obsolete=False to keep compatibility
  • when an environment is created, we save the python and build version in the environment.json file
  • if the environment.json file exist we use that versions to compare against the ones that the current version need to be used, if any of them differs, we wipe the environment
  • the environment.json is saved under each venv version so if one builder has one version and another build has another one, this doesn't interfers

Questions:

  1. do we need to treat conda environment in a special way?
  2. where tests for this should live? I need to see a similar test example to understand how to write them (I will need to mock json.load and other places)
  3. do I need to test something else than is_obsolete or that's enough?
@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 21, 2017

  1. I think conda shouldn't be effected by this.
  2. Probably here: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/rtd_tests/tests/test_doc_building.py#L33
  3. That's a good start for now.
)
@property
def is_obsolete(self):

This comment has been minimized.

@humitos

humitos Dec 22, 2017

Member

I wrote this at PythonEnvironment level, but @ericholscher said that conda won't be affected by this. Should I leave this here or move it to Virtualenv and write something customized by Conda?

This comment has been minimized.

@ericholscher

ericholscher Dec 22, 2017

Member

Honestly not sure. @willingc do you know anything about how conda will be effected by underlying docker image changes? My thought if that it's self-contained, but not really sure.

This comment has been minimized.

@willingc

willingc Dec 22, 2017

Contributor

Hmm... not sure. My understanding is docker images are built in layers so if you change an underlying layer it impacts the layers above.

This comment has been minimized.

@ericholscher

ericholscher Dec 22, 2017

Member

This would be changing out the underlying system python or conda version, I believe, is that main worry. We have the files on disk from a previous run, but we're hoping to let users change their docker image. We're planning to blow away the virtualenvs, because it breaks, but curious if conda will be effected.

This comment has been minimized.

@humitos

humitos Dec 23, 2017

Member

@willingc if a virtualenv is created with python 2.7.13 and we change the python version to 2.7.14, the venv stop working and need to be re-created. In the case of Conda we are not sure about that case but also the case that the conda environment was created with conda 4.x and then it's ran with conda 4.(x+1). What would happen in this case? Do you know?

ccing @Juanlu001 who was my Conda teacher :)

config=config,
)
env_json_data = '{"build": {"image": "readthedocs/build:2.0"}, "python": {"version": 3.5}}'
with patch('os.path.exists') as exists, patch('readthedocs.doc_builder.python_environments.open', mock_open(read_data=env_json_data)) as _open: # noqa

This comment has been minimized.

@humitos

humitos Dec 22, 2017

Member

These with patch() lines are terrible, but I didn't find a better way to write them :(

This comment has been minimized.

@stsewd

stsewd Dec 22, 2017

Member

It's fine to escape with backslash here according to pep8 https://www.python.org/dev/peps/pep-0008/#maximum-line-length

exists.return_value = True
self.assertTrue(python_env.is_obsolete)
@pytest.mark.xfail(reason='build.image is not being considered yet')

This comment has been minimized.

@humitos

humitos Dec 22, 2017

Member

to make this test pass, we need the PR that get the image from yaml merged... then we can remove this line and it should pass :)

@ericholscher

Looks like a great change to me!

"""Return the path to the ``environment.json`` file for this venv."""
return os.path.join(
self.venv_path(),
'environment.json',

This comment has been minimized.

@ericholscher

ericholscher Dec 22, 2017

Member

I do wonder if this should be namespaced a bit more (readthedocs-environment.json?) so that it doesn't conflict with anything in the future.

This comment has been minimized.

@humitos

humitos Dec 22, 2017

Member

Agree.

:rtype: bool
"""
# Always return True if we don't have information about what Python

This comment has been minimized.

@ericholscher

ericholscher Dec 22, 2017

Member

This returns False.

This comment has been minimized.

@humitos

humitos Dec 22, 2017

Member

Ha! I did it wrong at first, then I fix the return statement and forgot the comment :)

@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 27, 2017

Is this now clearing conda envs? If so, I think we're good to go.

@humitos

This comment has been minimized.

Member

humitos commented Dec 28, 2017

Is this now clearing conda envs? If so, I think we're good to go.

Yes. It's clearing the conda env if it's was created with a different python/build image version; but it's not checking the conda version.

I think it's should be a good start but maybe we will want to check the conda version also in the future --I'm not sure if that could affect or not, really.

@ericholscher

This comment has been minimized.

Member

ericholscher commented Dec 28, 2017

Great. Have you tested it locally w/ the docker image changes and virtualenv? If that works, it would be great to get this all merged so we can test it locally before deploying it next week.

@humitos

This comment has been minimized.

Member

humitos commented Dec 28, 2017

I tested this manually with the python version change from the YAML and docker image from project.container_image since using the YAML for the build image depends on #3339 and rtfd/readthedocs-build#33

Besides,

  • I just add a new commit with a change to make it py2 and py3 compatible when saving the json to disk.
  • I create a new method to get the python_full_version (making a refactor of the existent code) so we get and save the minor and major version to the json file
  • I added a test for changing the container_image from the Admin (at project level)

Finally, I just realized that the conda env is always deleted if its exist at these lines:

if os.path.exists(version_path):
# Re-create conda directory each time to keep fresh state
self._log('Removing existing conda directory')
shutil.rmtree(version_path)

Those lines were added at 475cf0b

So, if we are going to leave those lines there it seems useless to add the logic of this PR at PythonEnvironment class level, and should be moved to Virtualenv class I think.

Please, take a look again at these changes and give me your feedback.

@Juanlu001

This comment has been minimized.

Juanlu001 commented Dec 28, 2017

@ericholscher ericholscher merged commit 1a4bba1 into master Dec 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the humitos/wipe/automatically branch Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment