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.

Sign up for GitHub

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

Jump to bottom

Re-Organize Folders #717

Merged
merged 27 commits into from
Jan 30, 2025
Merged

Re-Organize Folders #717

merged 27 commits into from
Jan 30, 2025
+788 −806

Conversation

njbrake
Copy link
Contributor

@njbrake njbrake commented Jan 22, 2025 •
edited
Loading

What's changing

Re-organize the folder structure to more closely align with standard python library packaging.
After re-organization, the hardest part was fixing the links in the docs: they were hardcoded links to git commit hashes or sometimes the main branch. This was a latent issue (see #693) so this PR fixes that too. Now the sphinx build grabs the git commit and uses that to construct all the links, which is what we would want eventually anyways so that we could easily build the docs for any release and have it point to the correct files.

I also updated the github action "link checker" so that it is only responsible for checking the links outside of the docs folder. The github action link checker isn't compatible with the dynamic git commit insertion that is build into sphinx. Sphinx has a built in linkcheck extension which I activated, so we should still be all set with getting the links checked.

The old structure:

docs
lumigator
    frontend
    infra
        mzai
            helm
                lumigator
    python
        mzai
            backend
            jobs
            sample_data
            schemas
            sdk

The new structure I am proposing:

docs
infra
    helm
        lumigator
lumigator
    frontend
    backend
    jobs
    sample_data
    schemas
    sdk

I already...

  • [x ] Tested the changes in a working environment to ensure they work as expected
  • Added some tests for any new functionality
  • [x ] Updated the documentation (both comments in code and product documentation under /docs)
  • [ x] Checked if a (backend) DB migration step was required and included it if required

@github-actions github-actions bot added documentation Improvements or additions to documentation gha GitHub actions related labels Jan 22, 2025
@njbrake njbrake marked this pull request as ready for review January 23, 2025 01:23
@javiermtorres
Copy link
Contributor

Can you please provide a short summary of the relocations?

@njbrake njbrake linked an issue Jan 24, 2025 that may be closed by this pull request
Directory Structure Re-organization #742
Closed
@njbrake njbrake requested review from ividal and veekaybee January 28, 2025 15:54
@njbrake njbrake requested a review from agpituk January 28, 2025 15:54
@veekaybee
Copy link
Member

This looks ok to me at a high-level glance although it's fairly large (as we anticipated) 😅 . Is there an easy way to logically separate out the "renaming" aspect from fixing the links in the google docs? If possible, it would be good to have as two PRs.

@njbrake
Copy link
Contributor Author

njbrake commented Jan 28, 2025

This looks ok to me at a high-level glance although it's fairly large (as we anticipated) 😅 . Is there an easy way to logically separate out the "renaming" aspect from fixing the links in the google docs? If possible, it would be good to have as two PRs.

Not really: since the links part is currently tied to main branch, the CI would fail if I just updated the paths. For example:

docs/source/conceptual-guides/endpoints.md currently has a link to Schemas](https://github.com/mozilla-ai/lumigator/tree/b1ea63ba3e1aae5907e46ffbe9bfd809253c6053/lumigator/python/mzai/schemas/schemas). This is bad because it's linking to a hash that is disconnected from the version of the code that the docs is built for. I have three options:

  1. I fix the bad link but keep the hardcoding: If I update that code in this PR to be

**Schemas**](https://github.com/mozilla-ai/lumigator/tree/main/lumigator/schemas/schemas) then it will break because the new file structure doesn't exist on main, so CI will be broken but then I'll have to trust that it will work when that file does exist on main

  1. I update to point it to main but keep the hardcoding

**Schemas**](https://github.com/mozilla-ai/lumigator/tree/main/lumigator/python/mzai/schemas/schemas) this will work in my PR CI buiild, but then will cause a build failure as soon as I merge it to main, because that file path will no longer exist once the merge is finished, oh no!

  1. I fix the hard coding and the bad link
    This is the solution I went with: it makes it so that our docs don't point to the wrong version of code, and also makes it so that our CI builds will pass both on the PR branch as well as when we merge to main.

@njbrake njbrake requested a review from chainlink January 28, 2025 16:30
veekaybee
veekaybee approved these changes Jan 28, 2025
View reviewed changes
Copy link
Member

@veekaybee veekaybee left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks ok from the backend perspective. Thanks for taking this pass! Would love to get a few more eyeballs since this is a large one.

chainlink
chainlink approved these changes Jan 30, 2025
View reviewed changes
Copy link
Member

@chainlink chainlink left a comment

Choose a reason for hiding this comment

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

Tested on kube, looks good

@njbrake njbrake merged commit 77c0931 into main Jan 30, 2025
15 checks passed
@njbrake njbrake deleted the brake/reorg_folder branch January 30, 2025 13:36
@agpituk agpituk mentioned this pull request Feb 4, 2025
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@macaab26 macaab26 macaab26 approved these changes

@javiermtorres javiermtorres javiermtorres approved these changes

@veekaybee veekaybee veekaybee approved these changes

@chainlink chainlink chainlink approved these changes

@ividal ividal Awaiting requested review from ividal

@agpituk agpituk Awaiting requested review from agpituk

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation frontend gha GitHub actions related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Directory Structure Re-organization
5 participants
@njbrake @javiermtorres @veekaybee @chainlink @macaab26