DiamondLightSource / python3-pip-skeleton

Archived in favour of https://github.com/DiamondLightSource/python-copier-template
Apache License 2.0
5 stars 5 forks source link

Review #75

Closed gilesknap closed 1 year ago

gilesknap commented 2 years ago

A review meeting on 2 Nov 2022 decided on the following changes:

gilesknap commented 2 years ago

@coretl please review and edit the above. Thanks.

gilesknap commented 2 years ago

Update after later meeting with Martin.

dependencies are

gilesknap commented 2 years ago

@coretl

still have the container step create requirements.txt

This means there is no requirements.txt for people that remove Dockerfile.

coretl commented 2 years ago

still have the container step create requirements.txt

This means there is no requirements.txt for people that remove Dockerfile.

I think that's fine, the container and requirements.txt are both for deploying applications, if you aren't an application you don't need either

gilesknap commented 2 years ago

As part of this I'm going to look at Garry's approach to use of testpypi in this project https://github.com/DiamondLightSource/numcertain

gilesknap commented 2 years ago

@garryod I note that you are using

In numcertain.

We are considering dropping .vscode and .devcontainer.json from skeleton because we are recommending the use of a workspace project such as https://github.com/epics-containers/dev-u22-workspace for working with python projects.

What is your view? Did these files provide a starting point for what you now have in numcertain? would you prefer to keep them?

garryod commented 2 years ago

still have the container step create requirements.txt

This means there is no requirements.txt for people that remove Dockerfile.

I think that's fine, the container and requirements.txt are both for deploying applications, if you aren't an application you don't need either

The requirements.txt file serves as a useful artifact for debugging failures observed in the regular "checkup" workflows, IMO it would be nice to have even for library code

garryod commented 2 years ago

@garryod I note that you are using

  • .vscode
  • .devcontainer (folder)

In numcertain.

We are considering dropping .vscode and .devcontainer.json from skeleton because we are recommending the use of a workspace project such as https://github.com/epics-containers/dev-u22-workspace for working with python projects.

What is your view? Did these files provide a starting point for what you now have in numcertain? would you prefer to keep them?

The vast majority of my projects require some form of system dependency - whether it be the CUDA toolkit, some specific flavour of MPI, or some C library - and therefore require their own Dockerfile. A lot of these are at best difficult to get working in tandem and at worst fundamentally incompatible, so a single development container per project has made sense.

Further, it is nice that the contributor instructions can be as simple as "clone this repo and open in a vscode dev container", especially when collaborating with folks at different institutions

gilesknap commented 2 years ago

update: twine check in the dist step instead of using testpypi because: its one less token to generate and gives just as much info.

coretl commented 2 years ago

still have the container step create requirements.txt

This means there is no requirements.txt for people that remove Dockerfile.

I think that's fine, the container and requirements.txt are both for deploying applications, if you aren't an application you don't need either

The requirements.txt file serves as a useful artifact for debugging failures observed in the regular "checkup" workflows, IMO it would be nice to have even for library code

But you'd still get dev_requirements_pyx.x.txt files for each test run and lint, are those sufficient?

gilesknap commented 2 years ago

thanks @garryod

After further discussion we are going to keep .devcontainer and .vscode. We will make the settings minimal for working with the project (e.g. no extensions except pylance).

Then adopters can add to these or delete them as they see fit.

garryod commented 2 years ago

update: twine check in the dist step instead of using testpypi because: its one less token to generate and gives just as much info.

  • this does not check tags which is OK - if we are tagged badly then the actual push to pypi will fail anyway

Hadn't realised this was a thing, sounds ideal!

gilesknap commented 1 year ago

@garryod further to the test pypi conversation.

What kind of failures are you looking to catch with this? Thanks.

garryod commented 1 year ago

@garryod further to the test pypi conversation.

What kind of failures are you looking to catch with this? Thanks.

I've previously had it catch duplicate wheels for C extension builds, I would expect it to be a lot more useful when dealing with a large number of related files like with a C extension.

Also, as discussed on #72, twine check is fairly limited in what it verifies (only that stuff will be rendered correctly afaik)