Skip to content

Conversation

@ric-evans
Copy link
Member

@ric-evans ric-evans commented Apr 10, 2023

Adds package infrastructure for Python, CI, and PyPI.

Nearly all files are mostly empty. A follow-up PR will migrate files from https://github.com/icecube/skymap_scanner.

@ric-evans ric-evans added the enhancement New feature or request label Apr 10, 2023
@ric-evans ric-evans self-assigned this Apr 10, 2023
@ric-evans
Copy link
Member Author

@tianluyuan @mlincett what do we think about this directory/file structure?

@ric-evans ric-evans marked this pull request as ready for review April 11, 2023 15:26
@ric-evans ric-evans changed the title First PR Package Infrastructure Apr 11, 2023
Copy link
Contributor

@tianluyuan tianluyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I wonder if scripts should be at top level rather than module level?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be in the top level directory or inside the project?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I'd like to be able to solely rely on pip (as opposed to git cloning). Python/PyPI packaging does support executable scripts, but I'll need to look into this further.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PyPI does support executables and package data, as you can see with https://github.com/WIPACrepo/iceprod/blob/master/setup.py

But not sure I'd recommend it. My thoughts:

  1. if it's only an example, leave it in github or the docs
  2. if it will be common use, I like the pattern of using python -m skyreader.plots

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was planning to put https://github.com/icecube/skymap_scanner/blob/main/resources/utils/plot_skydriver_scan_result.py in scripts/, but this is simple enough to be an example/doc. I can't say for sure about https://github.com/icecube/skymap_scanner/blob/main/resources/utils/i3_to_json.py, I think this could be done with python -m utils.i3_to_json. What do you think @tianluyuan?

There is also https://github.com/icecube/[skywriter](https://github.com/icecube/skywriter), which may be better for script-like modules (like i3_to_json.py).

Copy link
Contributor

@mlincett mlincett Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems there is no general consensus on a best practice in relation to this, but maybe we could consider a src layout? It seems to force some more robustness in the workflow, however I remember editable installations with it being a bit tricky (no wait, I think that was about pyproject.toml).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beware the rabbit hole that is Python packaging ⚠️ There's never been a consensus in the Python community on this, and it seems there never will be--at least not any time soon.

But yeah, the idea is to separate "scripts" from modules. So to that point, we can split the two... commit incoming

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't say for sure about https://github.com/icecube/skymap_scanner/blob/main/resources/utils/i3_to_json.py, I think this could be done with python -m utils.i3_to_json. What do you think @tianluyuan?

There is also [https://github.com/icecube/skywriter](https://github.com/icecube/%5Bskywriter%5D(https://github.com/icecube/skywriter)), which may be better for script-like modules (like i3_to_json.py).

Probably it would make sense to go in skywriter since it's not meant to handle SkyScanResult? I'm flexible on the usage though a quick search turned up this which seems to indicate having a scripts or bin directory top level and pointing to it in setup.py would install it into your PATH. That might make sense for these cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put the scripts in skywriter, that's a fine solution

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No icetray requirements right? Lightweight would be nice 😎

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think we should make icetray requirements optional, at least to use SkyScanResult. Do you think that's doable?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can for sure migrate to i3astropy for coordinates conversion and I cannot think of other reasons we would require IceTray right now. Worst case scenario we can specify dependency groups no?

Copy link
Member Author

@ric-evans ric-evans Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would be the general idea. However, I'm pretty sure it wouldn't be as straightforward as extras (dependency groups) since icetray isn't a pip-installable package.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's no dependence on icetray at the moment already so it should definitely be doable for now. If we wanted to send back I3Frames for a few pixels then I'm not sure 😅 but we don't have to decide that yet I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you need a release of i3astropy. If you adopt it, you will be the first users to my knowledge

@mlincett
Copy link
Contributor

Should we consider adopting pyproject.toml? I try to stick to that for new projects but I am not sure what everybody's else experience is.

@ric-evans
Copy link
Member Author

ric-evans commented Apr 11, 2023

Should we consider adopting pyproject.toml? I try to stick to that for new projects but I am not sure what everybody's else experience is.

When I looked at this last (~1 year ago), setup.cfg was the most robust method. But you're free to submit an issue to https://github.com/WIPACrepo/wipac-dev-py-setup-action 😄 This is how I've automated much of the packaging process.

@ric-evans ric-evans merged commit d249bd9 into main Apr 13, 2023
@ric-evans ric-evans deleted the init branch April 13, 2023 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants