skypilot icon indicating copy to clipboard operation
skypilot copied to clipboard

[config] remove omegaconf as dependency

Open SeungjinYang opened this issue 7 months ago • 2 comments

The only logic that needs to be implemented to remove omegaConf is the dotlist parsing logic. This actually turned out to be pretty simple to implement using yaml.safe_load().

Added some unit test to make sure the logic implemented to replace omegaConf still acts as expected.

Tested (run the relevant ones):

  • [x] Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • [x] Any manual or new tests for this PR (please specify below)

SeungjinYang avatar Apr 25 '25 22:04 SeungjinYang

I'm confident of the code changes, less confident of Dockerfile and docs/source/getting-started/installation.rst changes.

SeungjinYang avatar Apr 25 '25 22:04 SeungjinYang

The release pipeline also installs omegaconf. We need to remove it in this PR for consistency. Thanks.

zpoint avatar Apr 26 '25 05:04 zpoint

@zpoint, my comment is now hidden in resolved suggestions, but I'm curious why azure-cli is being installed because I don't see it used anywhere.

maresb avatar Apr 27 '25 11:04 maresb

@maresb The azure-cli is a dependency for Azure to work with SkyPilot. You can see many az commands used in the codebase.

If you're asking why we need uv pip install "azure-cli>=2.65.0", check #4414 for an explanation. That PR introduced uv pip install, and this line is a prerequisite for uv pip install to work with azure-cli in SkyPilot's dependencies.

zpoint avatar Apr 27 '25 14:04 zpoint

Thanks @zpoint for the reference to the explanation!

~It seems that now azure-cli has advanced to v2.71.0 and no longer requires prerelease dependencies. I'm guessing that because prereleases are no longer required, uv pip install skypilot[all] may work without the separate install command.~

maresb avatar Apr 27 '25 19:04 maresb

uv pip install skypilot[all] may work without the separate install command.

@maresb

Just tried, still not working. I think the issue is with the azure-keyvault-administration dependency chain from azure-cli when using uv

uv venv --seed ~/pypi-check-env
source ~/pypi-check-env/bin/activate
uv pip uninstall skypilot
uv pip install -e ".[all]"

Using Python 3.10.13 environment at: /Users/zepingguo/pypi-check-env
  × No solution found when resolving dependencies:
  ╰─▶ Because there is no version of azure-keyvault-administration==4.4.0b2 and azure-cli>=2.65.0 depends on azure-keyvault-administration==4.4.0b2, we can conclude that azure-cli>=2.65.0 cannot
      be used.
      And because only the following versions of azure-cli are available:
          azure-cli<=2.65.0
          azure-cli==2.66.0
          azure-cli==2.66.1
          azure-cli==2.67.0
          azure-cli==2.68.0
          azure-cli==2.69.0
          azure-cli==2.70.0
          azure-cli==2.71.0
      and skypilot[all]==1.0.0.dev0 depends on azure-cli>=2.65.0, we can conclude that skypilot[all]==1.0.0.dev0 cannot be used.
      And because only skypilot[all]==1.0.0.dev0 is available and you require skypilot[all], we can conclude that your requirements are unsatisfiable.

      hint: `azure-keyvault-administration` was requested with a pre-release marker (e.g., azure-keyvault-administration==4.4.0b2), but pre-releases weren't enabled (try: `--prerelease=allow`)
(pypi-check-env) (sky) ➜  skypilot git:(dev/zeping/remove_azure_dependency) 

zpoint avatar Apr 28 '25 02:04 zpoint

Yes, you are correct @zpoint, apologies for my confusion. I have updated my suggestions accordingly to readd the --prerelease allow flag.

You can check the dependencies for a particular version under info.requires_dist or similarly for the latest version, and disappointingly they use a lot of prerelease versions.

There is now an upstream issue for this at https://github.com/Azure/azure-cli/issues/31356.

While the CLI happens to be on PyPI, my impression is that installing from PyPI is not a supported installation method. Thus it may be realistic to imagine that the Azure CLI is not written in a language other than Python. Perhaps in a subsequent PR should consider removing the skypilot[azure] extra? (For example we could instead enforce the same version constraint via the value returned by az --version.) And instead of suggesting pip install skypilot[azure] in the error message, we would simply link to the official installation instructions.

maresb avatar Apr 28 '25 08:04 maresb

Yes, we should look into avoiding depending on azure-cli directly. I opened #5419 to track this.

cg505 avatar Apr 28 '25 18:04 cg505

oh actually @SeungjinYang could you revert the docs changes until this is released to stable? Most people looking at the docs will see the latest version.

cg505 avatar Apr 29 '25 19:04 cg505