Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: move Julia files to IFTPipeline.jl subdirectory #170

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

hollandjg
Copy link
Collaborator

@hollandjg hollandjg commented Oct 24, 2024

... to help clean up the repository. The goal is to have neatly separated directories with:

  • the Julia package (IFTPipeline.jl),
  • some python scripts,
  • the cylc workflow configurations.

The CLI file is intentionally ignored here, and will be moved and refactored in #174

Part of:

@hollandjg hollandjg mentioned this pull request Oct 28, 2024
31 tasks
@hollandjg hollandjg force-pushed the jghrefactor/A-gather-julia-package-in-new-directory branch from 6f53391 to 3797150 Compare October 28, 2024 17:52
@hollandjg hollandjg requested review from tdivoll, cpaniaguam, danielmwatkins and mkkim400 and removed request for tdivoll October 28, 2024 17:52
@hollandjg hollandjg force-pushed the jghrefactor/A-gather-julia-package-in-new-directory branch from 934c537 to 6a0c594 Compare October 28, 2024 17:58
@hollandjg hollandjg changed the title Refactor: Gather Julia project in IFTPipeline.jl subdirectory refactor: Gather Julia project in IFTPipeline.jl subdirectory Oct 28, 2024
@hollandjg hollandjg changed the title refactor: Gather Julia project in IFTPipeline.jl subdirectory refactor: move Julia files to IFTPipeline.jl subdirectory Oct 28, 2024
@hollandjg hollandjg marked this pull request as ready for review October 28, 2024 18:42
@cpaniaguam
Copy link
Member

I appreciate the intention behind the proposed changes! However, I have some concerns about moving the core package files to a subfolder.

If we intend to publish this package to a registry (such as the Julia Registry for greater visibility), it’s essential that the core code files, README, Project.toml, supporting documentation, and License files remain at the root of the repository. This is a standard convention in the Julia ecosystem to ensure proper visibility and accessibility for users and registries alike.

Even if we don’t plan to add this project to a registry, pointing to a subfolder can complicate the installation process. Users would need to navigate to the repository page, click on the folder containing the code, and then copy the link to that folder. Alternatively, they would have to clone the entire repository and specify a subfolder for installation, which adds unnecessary complexity.

I believe these drawbacks outweigh the benefit of reducing the number of items in the root directory. What are your thoughts?

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

See comment.

@hollandjg
Copy link
Collaborator Author

I appreciate the intention behind the proposed changes! However, I have some concerns about moving the core package files to a subfolder.

Thanks for the feedback!

If we intend to publish this package to a registry (such as the Julia Registry for greater visibility), it’s essential that the core code files, README, Project.toml, supporting documentation, and License files remain at the root of the repository. This is a standard convention in the Julia ecosystem to ensure proper visibility and accessibility for users and registries alike.

The repo IFTPipeline.jl as it exists today isn't a clean Julia package. The Julia part is mixed up with a whole lot of other stuff, including the cylc configuration, the fetchdata.sh script, and so on, which aren't relevant for understanding the Julia part, and which we perhaps wouldn't want to be included in a registry. I want to make it easy to understand what is where.

It was really difficult for me to understand what was related and what could be ignored, and making this move helped me start to draw the boundaries between the different components. I think I understand now which Project.toml file is to be used for which thing.

Partitioning the code more cleanly is a necessary shim for the refactoring I'm doing. Whether once we've refactored and tested the code we move it back so it's more Julian is a different question. I'm happy to put that in as an issue.

Even if we don’t plan to add this project to a registry, pointing to a subfolder can complicate the installation process. Users would need to navigate to the repository page, click on the folder containing the code, and then copy the link to that folder. Alternatively, they would have to clone the entire repository and specify a subfolder for installation, which adds unnecessary complexity.

I don't think users beyond the research group are going to be using this version, because it's so hard to get the Python dependencies working, and it's not been really tested yet. I think we need to provide Docker images which can deal with the complicated Conda dependencies. I'd be happy to refactor it again later to help those users.

It looks like Pkg supports Pkg.add(PackageSpec(path="MainRepo", subdir="SubDir.jl")) now: JuliaLang/Pkg.jl#1422
... but I'm not sure if this feature has seen widespread adoption.

Some more things to think about, and let's assume that we keep the original structure IFTPipeline.jl/src:

  • In feat: add a new setup script for the Conda environment #177 we're going to add another subpackage for the Python setup, which needs to be run independently. It's going to be a mess if we have a directory structure like IFTPipeline.jl/PythonSetup.jl. Ideally we'd get rid of the python dependency completely and get rid of that problem, but it's a problem we have it right now. Having a separate subdirectory is a good way to solve that for the minute, whilst we're refactoring.
  • In feat: update SOIT to use PIPX and an independent Dockerfile #175 we're going to add a separate Python package as well. That would mean we have a subdirectory in the Julia package which is itself a python package, but it won't be related to the Julia code in any way – it isn't a dependency. That seems messy. A subdirectory for the Julia package makes that more understandable.

I believe these drawbacks outweigh the benefit of reducing the number of items in the root directory. What are your thoughts?

Here are the options I think we need to decide between (in my descending order of preference):

  • Live with a subdirectory for now (because it helps me organize my thoughts around this code, and I'm doing the refactoring), and restructure again in future to something which is a bit more Julian.
  • Merge IFTPipeline.jl into IceFloeTracker.jl. The one is pretty much needed for the other and we could make a single Docker container based on that repo for the IFT CLI (as with refactor: update Dockerfile for IFTPipeline.jl #174). The only reason I'm not suggesting we do that is that we don't have time right now.
  • Make a separate IFTPipeline.jl repo.
  • Don't have a subdirectory, and just try to reorganize other parts of the repo to make the boundaries clearer (I don't like this option because can't see a good way to do that and we're on a tight schedule).

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Live with a subdirectory for now (because it helps me organize my thoughts around this code, and I'm doing the refactoring), and restructure again in future to something which is a bit more Julian

I don't mind taking a temporary measure to facilitate development. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants