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

Tox4 #163

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

Tox4 #163

wants to merge 40 commits into from

Conversation

tibortakacs
Copy link
Collaborator

@tibortakacs tibortakacs commented Feb 21, 2023

This PR introduces tox4 support.

Notes:

  • The tests can be run only outside of tox.
  • The previous API has been followed, but there is not tox3 support.

tox_conda/plugin.py Outdated Show resolved Hide resolved
tox_conda/plugin.py Outdated Show resolved Hide resolved
@tibortakacs
Copy link
Collaborator Author

@drdavella @AntoineD I would be happy to move forward with this PR. I would like to get your initial feedback though. What do you think the next sensible steps should be?

@AntoineD
Copy link
Collaborator

AntoineD commented May 6, 2023

@tibortakacs How about having working tests?

@tibortakacs
Copy link
Collaborator Author

@AntoineD Sounds great for me. Which way do you prefer: having more complex code and tests but both tox3 and tox4 support, or the new major version is only for tox4 support?

@AntoineD
Copy link
Collaborator

AntoineD commented May 6, 2023

I think the tests should only support tox 4.

@tibortakacs
Copy link
Collaborator Author

@AntoineD Re-reading my previous post, it was slightly confusing: I meant whether the tox-conda library itself after this PR should support both tox3 and tox4, or just tox4?

@AntoineD
Copy link
Collaborator

AntoineD commented May 6, 2023

It should only support tox 4 for everything.

@dcrosta dcrosta removed their request for review May 8, 2023 16:28
@tibortakacs tibortakacs marked this pull request as ready for review June 2, 2023 23:14
@tibortakacs
Copy link
Collaborator Author

tibortakacs commented Jun 2, 2023

@AntoineD I updated all the tests to pass. I mainly kept the tests and the logic, but of course I had to update a few of them. I used the tox.pytest infrastructure. All tests are passing locally, but unfortunately I could not make it work with tox. I suspect that since the tests call tox_run from tox, there is a conflict somewhere, and despite all my effort it does not use the temporary directory that it should. (@gaborbernat You might have some advice here.)

I am not sure when I can spend another big push on this. It works well on my project (it has been used for months now, however, in a quite simple conda context). Of course, I am happy to align with your advice, but we might consider executing the tests purely from a virtualenv (all conda calls are mocked), and we can resolve this later.

@AntoineD What do you think?

@AntoineD
Copy link
Collaborator

AntoineD commented Jun 6, 2023

I am not familiar with tox 4 API so I let @gaborbernat decide what is the best.

def _get_python(self, base_python: List[str]) -> Optional[PythonInfo]:
exe_path = base_python[0]

output = self._run_pure(
Copy link
Member

Choose a reason for hiding this comment

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

Shold be cached to do fresh run every time 🤔 something like virtualenv does

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean we should run this only once, and cache the result?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great idea, I will do it.

return PythonInfo(implementation, version_info, version, is_64, platform_name, extra)

@property
def _package_tox_env_type(self) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we use virtualenv for packaging and not conda?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not see any urgent need to diverge from the original tox behavior here, I have not experienced any issue by using it in conda. But happy to change if there is an issue with this. What would be preferred command here?

Copy link
Member

Choose a reason for hiding this comment

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

Duno, would make some sense to use conda environments universally rather than virtualenv in some places and conda in others. nitpick. Ultimately I'm only advisory here. @AntoineD is the maintainer of this project, if he no longer needs to do so, I'm happy to hand over the maintainership bit to you.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good question. Happy to dig deeper how conda does the packaging, but it might take slightly longer. I will invest some time already in this PR to estimate how much work would be needed. If not too much, happy to do it now, if it took longer, I might prefer merging as it is not, and resolve it later. This is just an opinion though, happy to do either way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Duno, would make some sense to use conda environments universally rather than virtualenv in some places and conda in others. nitpick. Ultimately I'm only advisory here.

I think that packaging with anaconda should be opt in because:

  • some people use anaconda to get the required deps in constrained IT envs, but with no actual need for anaconda packaging.
  • building an anaconda package take way more time! (the way tox and conda build work have some overlap)

@AntoineD is the maintainer of this project, if he no longer needs to do so, I'm happy to hand over the maintainership bit to you.

I've been wondering about that for quite some time: I not longer have enough spare time for tox-conda and as I said do not know tox 4 so I guess that tox-conda deserves a better maintainer, @tibortakacs you seem to be the best candidate!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the insight, @AntoineD. Then, if you do not have any objection, let's keep the pip builder in this PR, and further down the line we can create a new option to enable conda build packager.

@AntoineD @gaborbernat Thank you, this is a great honor. Both tox and conda are key parts of the Python, especially in R&D communities, and tox-conda is the crucial glue between them. I am happy to act as the maintainer of the project, and see whether I can bear this responsibility on the level that this project deserves.


_, conda_deps = self.conf["conda_deps"].unroll()
if conda_deps:
conda_dict["deps"] = conda_deps
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need deps for Python?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This ensures that if the conda dependencies change, the environment is recreated because there will a cache change. Was the question about this?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be already covered by deps durings the setup, the same way the tox uses it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably you are right, I am still learning how tox does the job internally. Can you point me to the tox code to be sure that I am checking the right approach.

assert fnmatch(
pip_install_command,
(
f"*conda run -p {str(proj.path / '.tox' / env_name)} --live-stream"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gaborbernat Let me ask your advise here. All the tests are based on the fixtures (tox_project) that are copy (with minor modification) from the tox.pytest module. It works properly, when I call pytest directly, all tests are passing. However, when I use tox, it does not work well, proj.path point to the source root folder instead of the temporary directory. I am leaning towards running tox-conda test using pytest for now, but I do not particularly like this, I want to keep tox here. Is there any solution for this?

Copy link
Member

Choose a reason for hiding this comment

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

Would have to check why the path differs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is actually more, tox_project creates the tox environment in the .tox folder of the source folder.

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.

3 participants