nerfstudio-project / nerfstudio

A collaboration friendly studio for NeRFs
https://docs.nerf.studio
Apache License 2.0
9.34k stars 1.26k forks source link

Make `awscli` an optional dependency #3269

Closed pculbertson-bdai closed 2 months ago

pculbertson-bdai commented 3 months ago

Is your feature request related to a problem? Please describe. I'm hoping to use nerfstudio in a larger project, and awscli is causing lots of versioning conflicts. In particular, they version docutils<0.17 which prevents us from installing newer versions of sphinx, etc.

Would it be possible to make awscli an optional dependency, like the docs or generative modeling tools?

awscli is only used in the Eyeful Tower dataset download (which our project does not need) and it'd be great to have nerfstudio install without introducing strict versioning on our documentation.

Describe the solution you'd like Make awscli an optional dependency of nerfstudio, installed only by users who want to use the Eyeful Tower dataset.

Describe alternatives you've considered

Additional context N/A

brentyi commented 3 months ago

Hi Preston!

This is fine with me. Do you have time for a PR? Maybe if awscli isn't installed and ns-download-data is run for Eyeful Tower we can just print a message asking users to install it.

We do something similar for projectaria_tools: https://github.com/nerfstudio-project/nerfstudio/blob/d6df39b70d2e0b444178d23b694989e39d624781/nerfstudio/scripts/datasets/process_project_aria.py#L27-L34

cc @VasuAgrawal @ethanweber

pculbertson-bdai commented 3 months ago

Amazing -- yes, very happy to open a PR! I'll follow this pattern and open something likely later today.

VasuAgrawal commented 3 months ago

Hi @pculbertson-bdai , @brentyi ,

I was wondering if / when this would come back to bite us. I pointed out a similar issue regarding the old dependencies in my original PR. The issue at the time was only on Windows - sounds like it's a more general issue now?

I'm not opposed to making it an optional dependency (though a little sad that we have to because AWS doesn't want to provide packages for v2, and doesn't want to update v1 ...). I did try adding a direct dependency on the git tree for v2 with this:

python -m pip install 'awscli @ git+https://github.com/aws/aws-cli@v2'

but unfortunately that throws a bunch of errors since I don't have an MSVC toolchain installed on my laptop where I'm trying this (see below). Maybe we can consider using the unofficial awscliv2 instead? It seems to have been updated recently (last couple months), which is a positive sign. @pculbertson-bdai , if you have time, maybe you could try switching to awscliv2 (and updating the name of the executable in the download script) first?

(test_aws2) PS C:\Users\vasuagrawal\Documents\scratch> python -m pip install 'awscli @ git+https://github.com/aws/aws-cli@v2'
Collecting awscli@ git+https://github.com/aws/aws-cli@v2
  Cloning https://github.com/aws/aws-cli (to revision v2) to c:\users\vasuagrawal\appdata\local\temp\pip-install-1iijpj43\awscli_05090b58d15a44eaad9026a9e32f1dd2
  Running command git clone --filter=blob:none --quiet https://github.com/aws/aws-cli 'C:\Users\vasuagrawal\AppData\Local\Temp\pip-install-1iijpj43\awscli_05090b58d15a44eaad9026a9e32f1dd2'
  Running command git checkout -b v2 --track origin/v2
  Branch 'v2' set up to track remote branch 'v2' from 'origin'.
  Switched to a new branch 'v2'
  Resolved https://github.com/aws/aws-cli to commit 865cefc0e1837e364a28a7d4dc91cb4234fd3070
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... error
  error: subprocess-exited-with-error

  × pip subprocess to install backend dependencies did not run successfully.
  │ exit code: 1
  ╰─> [50 lines of output]
      Collecting cryptography<40.0.2,>=3.3.2
        Downloading cryptography-40.0.1-cp36-abi3-win_amd64.whl.metadata (5.6 kB)
      Collecting urllib3<1.27,>=1.25.4
        Downloading urllib3-1.26.19-py2.py3-none-any.whl.metadata (49 kB)
           -------------------------------------- 49.3/49.3 kB 501.0 kB/s eta 0:00:00
      Collecting awscrt<=0.20.11,>=0.19.18
        Downloading awscrt-0.20.11-cp311-abi3-win_amd64.whl.metadata (3.4 kB)
      Collecting python-dateutil<=2.8.2,>=2.1
        Downloading python_dateutil-2.8.2-py2.py3-none-any.whl.metadata (8.2 kB)
      Collecting ruamel.yaml<=0.17.21,>=0.15.0
        Downloading ruamel.yaml-0.17.21-py3-none-any.whl.metadata (13 kB)
      Collecting ruamel.yaml.clib<=0.2.7,>=0.2.0
        Downloading ruamel.yaml.clib-0.2.7.tar.gz (182 kB)
           -------------------------------------- 182.5/182.5 kB 5.6 MB/s eta 0:00:00
        Installing build dependencies: started
        Installing build dependencies: finished with status 'done'
        Getting requirements to build wheel: started
        Getting requirements to build wheel: finished with status 'error'
        error: subprocess-exited-with-error

        Getting requirements to build wheel did not run successfully.
        exit code: 1

        [12 lines of output]
        <string>:81: DeprecationWarning: ast.Str is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:81: DeprecationWarning: ast.Num is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:81: DeprecationWarning: ast.Bytes is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:81: DeprecationWarning: ast.NameConstant is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:103: DeprecationWarning: ast.Str is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:107: DeprecationWarning: ast.Bytes is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:109: DeprecationWarning: ast.Num is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:119: DeprecationWarning: ast.NameConstant is deprecated and will be removed in Python 3.14; use ast.Constant instead
        <string>:106: DeprecationWarning: Attribute s is deprecated and will be removed in Python 3.14; use value instead
        <string>:110: DeprecationWarning: Attribute n is deprecated and will be removed in Python 3.14; use value instead
        sys.argv ['setup.py', 'egg_info']
        test compiling C:\Users\VASUAG~1\AppData\Local\Temp\tmp_ruamel_es5kn28c\test_ruamel_yaml.c -> test_ruamel_yaml Exception: Microsoft Visual C++ 14.0 or greater is required. Get it with "Microsoft C++ Build Tools": https://visualstudio.microsoft.com/visual-cpp-build-tools/
        [end of output]

        note: This error originates from a subprocess, and is likely not a problem with pip.
      error: subprocess-exited-with-error

      Getting requirements to build wheel did not run successfully.
      exit code: 1

      See above for output.

      note: This error originates from a subprocess, and is likely not a problem with pip.

      [notice] A new release of pip is available: 24.0 -> 24.1.1
      [notice] To update, run: python.exe -m pip install --upgrade pip
      [end of output]

  note: This error originates from a subprocess, and is likely not a problem with pip.
error: subprocess-exited-with-error

× pip subprocess to install backend dependencies did not run successfully.
│ exit code: 1
╰─> See above for output.

note: This error originates from a subprocess, and is likely not a problem with pip.

[notice] A new release of pip is available: 24.0 -> 24.1.1
[notice] To update, run: python.exe -m pip install --up^C
pculbertson-bdai commented 3 months ago

@VasuAgrawal I can first try just dropping in their v2 CLI and seeing if it works, but I don't think I have time to refactor the Eyeful dataset code (it's not super relevant to my project) if it's complicated.

I definitely agree that using the newer awscli would be a more permanent fix. Their strict dependencies + decision to include their documentation deps in their pypi release is pretty frustrating.

I'll keep you posted on how it goes + if it's a hassle, I'll plan to PR the original change (making awscli optional) until someone who works with Eyeful has time to work on it more thoroughly.

VasuAgrawal commented 3 months ago

@pculbertson-bdai thanks for taking a crack at it! At a glance, I think it's a fairly simple change, just changing a couple imports to use the unofficial awscliv2, and replacing the usage of driver.main(command) with aws_api.execute(command). If you run into trouble, we can do the original PR you had intended, and I can take a crack at it afterwards.

pculbertson-bdai commented 3 months ago

Actually @brentyi looking at how you guys manage the dependencies for the various custom data formats (e.g., Project Aria, Spectacular AI), I think the original fix might make more sense from a project maintenance standpoint?

For example, my project doesn't use Eyeful, but having awscli-v2 in the nerfstudio dependencies will prevent anyone on my team from using v1.

Having users install dataset-specific dependencies separately from the core nerfstudio might actually make more sense in the long term, especially since people are constantly adding new datasets to the repo -- if they're all adding their deps to the core requirements.txt then eventually it will be difficult to install nerfstudio side-by-side with anything else.

So -- I'm happy to try fixing this one dependency clash, but think a better solution might be our original fix. Do y'all have any thoughts?

brentyi commented 3 months ago

For example, my project doesn't use Eyeful, but having awscli-v2 in the nerfstudio dependencies will prevent anyone on my team from using v1.

Yeah, given this and the fact that most nerfstudio users won't need to download the Eyeful Tower dataset, doing the original fix first seems simplest/safest to me!

I also want to respect your time here, since it looks like an awscliv2 switch could be a longer conversation. (about v1 / v2 conflicts like you mentioned, testing, that the awscliv2 pip package is unofficial and only being begrudgingly maintained which gives me some hesitation)