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

Fix get_build_dir when custom_build is enabled #1500

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

Conversation

lamylio
Copy link

@lamylio lamylio commented Nov 5, 2024

Hello,

I've noticed that when custom_build is set in the configuration, the get_build_dir function doesn’t use local_target or packaged_target at all, causing Chainlit to return the same build path for both build_dir and copilot_build_dir. As a result, the static mount "/copilot" (server.py, line 225) does not point to the copilot directory, and the /copilot/index.js route doesn’t exist, making the copilot unavailable.

Currently, users can work around this by moving the copilot index.js file into the frontend’s dist folder. However, I believe it would be better to maintain the same structure as the one of the package, with separate folders for each component. Also, I don't feel it is necessary to have two separate (yet the same) parameters for the get_build_dir function.

Example structure:

- app
  - app.py 
  - .chainlit/
  - .files/
  - build/
    - copilot/
      - dist/
        - index.js 
    - frontend/
      - dist/
        - assets/*
        - index.html
        - favicon.svg

Before:
custom_build = "build" → doesn’t work at all
custom_build = "build/frontend/dist" → frontend works, copilot doesn’t (unless we move index.js to it)

After:
custom_build = "build" → both work (and seems more natural to me)

Feel free to modify the way I’ve implemented it if you see a better approach! :)
P.S. Haven't created any new tests, as at first glance the function didn't seem to be used elsewhere.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. backend Pertains to the Python backend. bug Something isn't working labels Nov 5, 2024
@dokterbob
Copy link
Collaborator

dokterbob commented Nov 6, 2024

Thanks for the contrib @lamylio!

What I noticed trying to figure out that get_build_dir() is supposed to do is that there's actually not a clear specification of what custom_build is supposed to do; it's not documented anywhere (at least where I could find), other than ''custom build directory for the frontend' in the config. Yet, it is a public API -- so any changes to it's behaviour are likely to get some of our users in trouble!

Going forward, I'd like to improve on this! Let's first get clear on what custom_build is supposed to do, that is, documenting the specific use case(s) and steps required to use it. Can be here, we can use the discussion in this ticket to write a documentation section.

Based on that, I would like to have the documented use case(s) unit tested (so get_build_dir()) and the custom_build E2E test or your patch should be adjusted so it will pass. Breaking the test implies that we're breaking compatibility, so we should really consider whether it is necessary and worthwhile.

If (and only if) we're already changing the API, we might as well rename custom_build to something like custom_frontend and, correspondingly, get_build_dir() to get_frontend_dir().

Alternatively: we have frontend builds run automatically now for GIT-based installs. That implies that developers can simply fork the chainlit repo and install directly from there -- so technically there is no more reason to support serving a custom frontend from a different location.

@willydouhard Curious what your thoughts are, and perhaps to have more insights into design decisions that lead to the current API.

@dokterbob dokterbob added the blocked Awaiting update or feedback from user after initial review/comments. label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Pertains to the Python backend. blocked Awaiting update or feedback from user after initial review/comments. bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants