IDAES / idaes-ui

User interfaces for IDAES
Other
1 stars 5 forks source link

Fix issue 36 pypi idaes-pse[ui] does not work with idaes-pse from GitHub main #39

Closed CopyDemon closed 11 months ago

CopyDemon commented 11 months ago

Link to issue: issue #36

Fixes

  1. delete old static UI files.
  2. update vite react build path to idaes_ui.fv.static
  3. build react static file to that path
  4. update model_server.py _static_dir _template_dir point to the static folder

Proposed changes: in PyPI package the model_server.py can't read static file from IDAES-UI.dist/ folder. To fix this update react build path point to the old static file folder in idaes_ui.fv.static/

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.
CopyDemon commented 11 months ago

@lbianchi-lbl this bug is because the python can't read from react's dist folder, but in old days it can read from idaes_ui.fv.static folder, so I have to remove all old static files and build react into that static folder, that is why there are lots of frontend code been added.

The CI works ok also let me find a way to download a test package try to check if the new build is works.

Last time its broken within package but works on local dev enc

lbianchi-lbl commented 11 months ago

@lbianchi-lbl this bug is because the python can't read from react's dist folder, but in old days it can read from idaes_ui.fv.static folder, so I have to remove all old static files and build react into that static folder, that is why there are lots of frontend code been added.

OK, thanks. I think I understand that we can expect many changes affecting frontend files. My question was more specifically about the font files (fonts/Open_Sans/... with .tff extension; roboto-... with .woff/.woff2 extension) being added. If those files were already being generated in the build, we could expect them to show up as "renamed" (from IDAES-UI/dist/... to idaes_ui/fv/static/...), but instead, they show up as "added", which suggests that they didn't exist at all in the previous IDAES-UI/dist directory.

The CI works ok also let me find a way to download a test package try to check if the new build is works.

Last time its broken within package but works on local dev enc

The CI checks test both types of installation: "dev", which is similar to what we had before, and "standard", which corresponds to a user installing pip install idaes-ui (or pip install "idaes-pse[ui]"). Since the tests pass, I'm reasonably confident the problems in #36 are addressed in this PR. That said, I agree with you that it's a good idea for you to check out the changes locally before merging to verify that the code still works with running the tests locally in your dev environment.

CopyDemon commented 11 months ago

@lbianchi-lbl this bug is because the python can't read from react's dist folder, but in old days it can read from idaes_ui.fv.static folder, so I have to remove all old static files and build react into that static folder, that is why there are lots of frontend code been added.

OK, thanks. I think I understand that we can expect many changes affecting frontend files. My question was more specifically about the font files (fonts/Open_Sans/... with .tff extension; roboto-... with .woff/.woff2 extension) being added. If those files were already being generated in the build, we could expect them to show up as "renamed" (from IDAES-UI/dist/... to idaes_ui/fv/static/...), but instead, they show up as "added", which suggests that they didn't exist at all in the previous IDAES-UI/dist directory.

The CI works ok also let me find a way to download a test package try to check if the new build is works. Last time its broken within package but works on local dev enc

The CI checks test both types of installation: "dev", which is similar to what we had before, and "standard", which corresponds to a user installing pip install idaes-ui (or pip install "idaes-pse[ui]"). Since the tests pass, I'm reasonably confident the problems in #36 are addressed in this PR. That said, I agree with you that it's a good idea for you to check out the changes locally before merging to verify that the code still works with running the tests locally in your dev environment.

In the original UI code, which wasn't based on React, the font files you mentioned didn't exist. These font files are part of the React-based UI, located in IDAES-UI/dist/assets and IDAES-UI/dist/assets/font/Open_Sans/static.

When I reconfigured the build process to place the output in the idaes_ui.fv.static folder, these font files were transferred there for the first time. Therefore, they appear as 'added' in our version control system, since they were not present in the idaes_ui.fv.static folder before this change.

lbianchi-lbl commented 11 months ago

In the original UI code, which wasn't based on React, the font files you mentioned didn't exist. These font files are part of the React-based UI, located in IDAES-UI/dist/assets and IDAES-UI/dist/assets/font/Open_Sans/static.

When I reconfigured the build process to place the output in the idaes_ui.fv.static folder, these font files were transferred there for the first time. Therefore, they appear as 'added' in our version control system, since they were not present in the idaes_ui.fv.static folder before this change.

@CopyDemon thanks for the clarification, I think I understand now. I expected the files to show up as "renamed", and the fact that they instead show up as "added" is because the files are still present in the original IDAES-UI/dist/assets directory.

My question is: do we want extra copies of the files that are now in idaes_ui/fv/static to be left in IDAES-UI/dist/assets? It's OK if you think we need both. However, if instead we don't need them, then I think it would be better to remove them since the added size (8 MB) is not negligible.

CopyDemon commented 11 months ago

In the original UI code, which wasn't based on React, the font files you mentioned didn't exist. These font files are part of the React-based UI, located in IDAES-UI/dist/assets and IDAES-UI/dist/assets/font/Open_Sans/static. When I reconfigured the build process to place the output in the idaes_ui.fv.static folder, these font files were transferred there for the first time. Therefore, they appear as 'added' in our version control system, since they were not present in the idaes_ui.fv.static folder before this change.

@CopyDemon thanks for the clarification, I think I understand now. I expected the files to show up as "renamed", and the fact that they instead show up as "added" is because the files are still present in the original IDAES-UI/dist/assets directory.

My question is: do we want extra copies of the files that are now in idaes_ui/fv/static to be left in IDAES-UI/dist/assets? It's OK if you think we need both. However, if instead we don't need them, then I think it would be better to remove them since the added size (8 MB) is not negligible.

I think we can ignore the entire IDAES-UI folder, the static folder is the build folder for react production. The user does not need to download the IDAES-UI. In the future, every time before release I will build the react manually into the static folder in idaes_ui.fv.static

lbianchi-lbl commented 11 months ago

@CopyDemon can you confirm that 86ec41f deletes the proper directory?

CopyDemon commented 11 months ago

@CopyDemon can you confirm that 86ec41f deletes the proper directory?

That commit is looks good to me, and that is the correct dir