nv-legate / legate.core

The Foundation for All Legate Libraries
https://docs.nvidia.com/legate/24.06/
Apache License 2.0
186 stars 61 forks source link

Use conda for all dependencies. #861

Closed trivialfis closed 11 months ago

trivialfis commented 11 months ago

For consistency and performance.

I use mamba for creating environments, which is much faster than the resolver from pip.

copy-pr-bot[bot] commented 11 months ago

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

manopapad commented 11 months ago

Asking @bryevdv to take a look, as he originally created the pip section for packages that were better suited for pip sourcing.

manopapad commented 11 months ago

/ok to test

bryevdv commented 11 months ago

better suited for pip sourcing

@manopapad No special concerns from me. If they were put in pip section originally then that's because there was some issue obtaining then via conda at the time. If the issues are now resolved then switching to using conda seems fine. (Would just acknowledge that if anything ever becomes difficult to obtain via conda it might need to get moved to pip in general).

Just one comment regarding moving some logic to the jinja template

manopapad commented 11 months ago

/ok to test

bryevdv commented 11 months ago

@trivialfis By "handle this in the template" I was imaginging surrounding the pip section with jijna {% if conditional to omit it entirely in case the list of dependencies was empty. Can you confirm that leaving

  - pip
  - pip:

with nothing after the pip: results in env files that still work? (It may very well, but I would not want to assume)

trivialfis commented 11 months ago

I have tested with mamba env update -f, but your suggestion is better, I will omit the pip section.

trivialfis commented 11 months ago

Looking into the code, I think the generator doesn't depend on jinja2, actually, it doesn't have any dependency since it's the script used to list dependencies in the first place. I think we might want to stay with an empty section or use conditions in the Python format function.

bryevdv commented 11 months ago

Oh right we had used jinja originally but then removed it, and I misled myself with the _TEMPLATE name. In that case yes your original code seems appropriate.

manopapad commented 11 months ago

So this is good to merge then?

bryevdv commented 11 months ago

No I think the going back to the original

pip=PIP_TEMPLATE.format(pip_sections=pip_sections)
            if pip_sections
            else "",

is probably the best idea.

trivialfis commented 11 months ago

No I think the going back to the original

Done.

manopapad commented 11 months ago

/ok to test