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

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

Merged
merged 12 commits into from Dec 28, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Dec 21, 2017

Initial drawing of ideas on how to implement #3190

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 21, 2017

Looks like a good start!

@humitos
Copy link
Member Author

@humitos 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
Copy link
Member

@ericholscher 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):
Copy link
Member Author

@humitos humitos Dec 22, 2017

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?

Copy link
Member

@ericholscher ericholscher Dec 22, 2017

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.

Copy link
Contributor

@willingc willingc Dec 22, 2017

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

Copy link
Member

@ericholscher ericholscher Dec 22, 2017

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.

Copy link
Member Author

@humitos humitos Dec 23, 2017

@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
Copy link
Member Author

@humitos humitos Dec 22, 2017

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

Copy link
Member

@stsewd stsewd Dec 22, 2017

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')
Copy link
Member Author

@humitos humitos Dec 22, 2017

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 :)

Copy link
Member

@ericholscher ericholscher left a comment

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',
Copy link
Member

@ericholscher ericholscher Dec 22, 2017

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.

Copy link
Member Author

@humitos humitos Dec 22, 2017

Agree.

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

@ericholscher ericholscher Dec 22, 2017

This returns False.

Copy link
Member Author

@humitos humitos Dec 22, 2017

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

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Dec 27, 2017

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

@humitos
Copy link
Member Author

@humitos 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
Copy link
Member

@ericholscher 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 humitos force-pushed the humitos/wipe/automatically branch from 3e464e2 to da1fde3 Dec 28, 2017
@humitos
Copy link
Member Author

@humitos 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 readthedocs/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:

https://github.com/rtfd/readthedocs.org/blob/da1fde31d1756cb3f2790f0f5c753c7c1c0cc595/readthedocs/doc_builder/python_environments.py#L295-L298

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.

@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Dec 28, 2017

@ericholscher ericholscher merged commit 1a4bba1 into master Dec 28, 2017
1 check passed
@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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants