Conversation
|
It's come up a couple of times, but there are good reasons not to use |
Ah okay fair enough. I'll modify it to stick with |
|
@copilot Fix the issue with the Windows CI. |
|
@joewallwork I've opened a new pull request, #566, to work on those changes. Once the pull request is ready, I'll request review from you. |
* Fix Windows CI: pass venv Python executable to CMake * Consistent capitalisation --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: joewallwork <22053413+joewallwork@users.noreply.github.com>
|
@jatkinson1000 this is now ready for re-review. I found an unrelated problem with the Windows CI that it wasn't actually running any tests because none of the |
jatkinson1000
left a comment
There was a problem hiding this comment.
Thanks @joewallwork this is amazing and will make life a lot easier going forwards!
A few small comments, some of which are nits so I think this is really close to merging.
pages/installation/general.md
Outdated
| - [CMake](https://cmake.org/) >= 3.18 | ||
| - Fortran (2008 standard compliant), C++ (must fully support C++17), and C compilers | ||
| - [LibTorch](https://pytorch.org/cppdocs/installing.html)[^1] or [PyTorch](https://pytorch.org/) | ||
| - TorchVision and NumPy (required for examples) |
There was a problem hiding this comment.
PyTorch also explicitly required for examples (not just LibTorch)?
As is mpi4py, matplotlib and more (mentioned below)...
Perhaps we don't specify modules here, or just provide "Other Python dependencies are required for examples/tests" and link to the below section?
There was a problem hiding this comment.
Addressing this in #570 by modernising the pip install approach.
| It provides the user with the option to [jit.script](https://pytorch.org/docs/stable/generated/torch.jit.script.html#torch.jit.script) or [jit.trace](https://pytorch.org/docs/stable/generated/torch.jit.trace.html#torch.jit.trace). | ||
|
|
||
| Dependencies: | ||
| - PyTorch |
There was a problem hiding this comment.
pip has some odd things to say here: https://pip.pypa.io/en/stable/cli/pip_install/#finding-packages
But happy to merge this if it seems to be working correctly.
There is no priority in the locations that are searched. Rather they are all checked, and the “best” match for the requirements (in terms of version number - see the specification for details) is selected.
Co-authored-by: Jack Atkinson <109271713+jatkinson1000@users.noreply.github.com>
jatkinson1000
left a comment
There was a problem hiding this comment.
Thanks @joewallwork
LGTM pending merge of #570 and two typos, thanks!
Co-authored-by: Jack Atkinson <109271713+jatkinson1000@users.noreply.github.com>
Closes #368
For a while I've thought that it'd be more appropriate to have
pt2tsas a command-line script that reads in a PyTorch model and writes out a TorchScript model. That's done in this PR.I feel that other "template" code that's currently in there would be better off existing in examples/integration tests. Some of the testing code can be included in the main
pt2tsscript. Further, I think we should drop the duplicatedpt2ts.pyfiles from the examples, move any specific testing part of them (underif __name__ == "__main__:") to the bottom of the corresponding net definition files (e.g.,simplenet.py), and make use of the newpt2tsscript in each example. (This has the bonus of testing it, which isn't currently done for the template inutils/pt2ts.py.)Note that I've made a recommendation to switch to a.tsextension for TorchScript files, although this is just personal preference.Proposed workplan
ftorch_utilspt2ts.pyscriptftorch_utilsup as a proper Python modulept2tsup as a script that gets installed in the Python environment.ftorch_utilsin the CIpt2ts.pyscripts found in the examples into the corresponding net definition filespt2ts.pyscript in the examplesI propose to defer the following to follow-up PRs:
ftorch_utilsmodule for a follow-up PR - see Unittestftorch_utilsPython module #563pt2tsto load pre-trained PyTorch models (needed for ResNet example) - see Allow pre-trained models withpt2ts#565