patrick-kidger / signatory

Differentiable computations of the signature and logsignature transforms, on both CPU and GPU. (ICLR 2021)
Apache License 2.0
261 stars 34 forks source link

v127 #45

Closed josephlmulligan closed 2 years ago

josephlmulligan commented 2 years ago

Added support for PyTorch 1.10.0, 1.10.1, 1.11.0 Dropped support for Python 3.6 Dropped support for Pytorch < 1.8.0

josephlmulligan commented 2 years ago

It seems GitHub removed support for Ubuntu 16.04, causing actions to not run. I have updated the workflows to use 18.04 instead.

patrick-kidger commented 2 years ago

Thanks for contributing! It looks like you've made precisely the changes needed.

It does look like the Windows jobs still aren't being picked up? (At time of writing.) I'm not sure what's up with that.

Two semi-related comments. Whilst we're making these changes, could you also (a) cut down on the number of builds a bit more (let's say remove PyTorch 1.8.0; 1.8.1; 1.9.0; 1.9.1) and (b) remove the deprecated macOS builds? I'm realising there's a possibility we bump up against the free usage limits going forward, given that I use Github Actions for other projects too.

Once things are straightened out and this PR is merged then I'll make sure the new release is pushed to PyPI.

josephlmulligan commented 2 years ago

I've tried to fix the windows problem but the error is a bit cryptic (see below), hopefully this works.

Run "C:/Program Files (x86)/Microsoft Visual Studio/201[7](https://github.com/patrick-kidger/signatory/runs/6623512522?check_suite_focus=true#step:4:8)/Enterprise/VC/Auxiliary/Build/vcvars64.bat" && SET DISTUTILS_USE_SDK=1 && %CONDA%/Scripts/conda create -n myenv python=%PYTHON_VERSION% -y && %CONDA%/Scripts/activate myenv && python -m pip install --upgrade pip && conda install pytorch==1.[8](https://github.com/patrick-kidger/signatory/runs/6623512522?check_suite_focus=true#step:4:9).0 cpuonly -c pytorch -y && python command.py should_not_import && python setup.py egg_info --tag-build=".1.8.0" bdist_wheel && python command.py should_not_import && for %%f in (./dist/*) do (python -m pip install ./dist/%%~nxf) && python -m pip install numpy && python -m pip install iisignature pytest && python -c "import os; import subprocess; import sys; print(sys.version); returncode_test = subprocess.Popen('python command.py test', shell=True).wait(); returncode_version = sys.version[:5] != os.environ['PYTHON_VERSION'][:5]; sys.exit(max(returncode_test, returncode_version)) " && echo done
The system cannot find the path specified.
Error: Process completed with exit code 1.

I've also made those changes regarding pytorch and macOS.

patrick-kidger commented 2 years ago

Hurrah! Merged. Give the bots a few hours to do their thing and it should be available on PyPI.

patrick-kidger commented 2 years ago

Unfortunately looks like the release bot failed. (Some kind of change to twine by the looks of it?) Do you want to try digging into it? Probably it's just a change to the appropriate CLI command.

josephlmulligan commented 2 years ago

It doesn't look like twine has had any changes, but it looks like the error might be that the PyPi password isn't getting added prior to the twine upload command being run. The error log shows the executed upload as:

twine upload -u patrick-kidger -p dist/signatory-1.2.7.1.11.0.tar.gz
usage: twine upload [-h] [-r REPOSITORY] [--repository-url REPOSITORY_URL]
                    [-s] [--sign-with SIGN_WITH] [-i IDENTITY] [-u USERNAME]
                    [-p PASSWORD] [--non-interactive] [-c COMMENT]
                    [--config-file CONFIG_FILE] [--skip-existing]
                    [--cert path] [--client-cert path] [--verbose]
                    [--disable-progress-bar]
                    dist [dist ...]
twine upload: error: the following arguments are required: dist

Which I think is trying to use dist/signatory-1.2.7.1.11.0.tar.gz as the password. Maybe take a look and see if the issue is with the secrets?

Also just an FYI - looks like I omitted updating the readme with the reduced number of PyTorch builds in the second pull request, let me know if you want me to send another PR or if you're able to change it while looking at the secrets.

patrick-kidger commented 2 years ago

Hmmm. Honestly I'm not sure what's going on with this.

I've double-checked that the appropriate secret is present. (And indeed it's been working all this time prior to this.)

And what I think is the relevant line in the deploy script seems to have things there: https://github.com/patrick-kidger/signatory/blob/2ce3b4af491dde96707b949528cdb648086dbce7/.github/workflows/deploy.yml#L62

Moreover the line printed in the logs seems fishy to me. IIRC GitHub Actions normally prints out something like **** in place of a secret, but here it hasn't printed anything at all. In addition it's printed out a filename in the dist, rather than the literal * written in the command.

Perhaps see if you can reproduce this error in a Github Action you control + with a dummy PyPI repository (consider using test.pypi.org) to pin down what the issue is? Looking at the diff the only thing that jumps out at me is the change in Ubuntu version, which might (might) explain things. Or it may be some underlying change in how Github Actions itself parses things. Or it's something else altogether.

Getting in this support is certainly proving to be harder than usual. I'm sorry about that. But conversely you can see how this kind of difficulty is why Signatory something of a maintenance burden.

nguyenntt97 commented 1 year ago

It's just my wild guess but I reviewed GitHub's action guide for encrypted secrets a bit—see GitHub docs

As @josephlmulligan mentioned the secret acquisition failed on runtime. The problem might be due to the syntax of referencing the secret directly in the command.

I could see there's a bit difference between your current syntax and the recommended syntax from GitHub. They seem to suggest declaring the secret from as an env variable first before using it in run.

steps:
  - name: Hello world action
    with: # Set the secret as an input
      super_secret: ${{ secrets.SuperSecret }}
    env: # Or as an environment variable
      super_secret: ${{ secrets.SuperSecret }}

I could see this difference had no effect in the past (last successful build with 1.2.6) but is there any chance there's a change in Action policy that prevented accessing secrets directly since then?

I tried searching for the relevant changelog but to no avail, so just hoping this could be a glue to fix your deployment.


Another possibility is SO thread. Attempting to retrieving environment secrets will be failed without referencing explicitly the environment name