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

Calyx website revamp wishlist #1926

Open
1 of 4 tasks
rachitnigam opened this issue Feb 17, 2024 · 4 comments
Open
1 of 4 tasks

Calyx website revamp wishlist #1926

rachitnigam opened this issue Feb 17, 2024 · 4 comments
Labels
Type: Tracker Track various tasks

Comments

@rachitnigam
Copy link
Contributor

rachitnigam commented Feb 17, 2024

Tracker for changes to the Calyx website:

  • New section for "papers" and "papers that use Calyx" (see egg page for example)
  • Easy to generate blog with RSS feed
  • Revamp the frontends section to provide more details and highlight Halide & CIRCT flows (C++ & PyTorch)
  • Fix the playground (Re-enable playground github workflow #1737)
@rachitnigam rachitnigam added the Type: Tracker Track various tasks label Feb 17, 2024
@rachitnigam rachitnigam changed the title Calyx website revamp wish-list Calyx website revamp wishlist Feb 17, 2024
@YJDoc2
Copy link
Contributor

YJDoc2 commented Feb 27, 2024

Hey, wrto Fix the playground :
When I go to the playground , and click compile on the default-selected options (Sequence example with top down compile control) , there is an error as below

Error:
import not supported in the web demo

I tried two other examples : Conditional and Loops, but both give same error. Is it due to the CI failing as you mentioned, or is it something else? And the CI link the the linked issue no longer exists, is there a newer version of failing CI?

Thanks :)

@sampsyo
Copy link
Contributor

sampsyo commented Feb 27, 2024

The summary, @YJDoc2, is essentially just bit-rot—yes, we have disabled automatic updates, so it's not getting synced to the most recent version of the Calyx compiler. Getting things working again could be a fun project for anyone interested in getting involved with the Calyx ecosystem. 😃

@YJDoc2
Copy link
Contributor

YJDoc2 commented Feb 28, 2024

Hey @sampsyo , thanks for your response! I tried playing around with the code, and got it to run properly on local, but I wanted to ask something regarding the changes I did :

  1. I have changed parcel to vite. parcel v1 is deprecated, and v2 does not have good/any support for loading wasm. I don't think the playground was using any parcel specific stuff apart from loading wasm, so it was a drop-in replacement for npm start , still need to check on the npm build.
  2. There was a bug in code due to which the error I reported was getting shown, so I fixed that. Another thing is, current code would run compile every time we changed compiler pass , which I changed to compile only when compile button is pressed. I observed that with original code, the page would hang up whenever I tried to select/de-select the passes.
  3. After solving the above error, I got a new one IO Error: operation not supported on this platform . After digging, I found that it originates from merge_namespace , which in turn originates from canonicalize_extern->canonicalize . As an experiment, I removed the canonicalize_extern call, and instead passed the original path directly to self.lib.add_extern(abs_path, exts); -> self.lib.add_extern(p.into(), exts); While this made the website work, I don't know if that is correct, or the code generated with it is correct or not. I think the canonicalize function has to do something with the import statements, but not sure. Can you help me with this, and tell me what the intent of this code is?
  4. If I'm correct in above; then as we don't need the import statements in the web (because we already resolve them beforehand) , would it be ok to do a target-based conditional compilation, where for wasm it will do p.into() and for other it will be as it is now?
  5. Also, I personally use npm instead of yarn, so it made a npm lockfile instead of yarn lockfile. Do you have a specific preference for yarn, or is it ok to remove the yarn lockfile and add the npm one instead? This mostly won't matter for local running, but for CI build, it is usually preferable to have a lockfile and do npm/yarn ci so that install is successfuly and does not accidentally update deps with potential to break.

If the above is fine, I'd be happy to clean up the changes I have and make a PR. I also have some other improvements in mind for the page, but can suggest those separately. Let me know. Thanks :)

@sampsyo
Copy link
Contributor

sampsyo commented Feb 29, 2024

Awesome!!! Thank you so much for looking into this; truly amazing. As you suggest, it would be great to review a PR—your proposed changes all sound excellent, so we can iterate on the details there.

I have changed parcel to vite. parcel v1 is deprecated, and v2 does not have good/any support for loading wasm. I don't think the playground was using any parcel specific stuff apart from loading wasm, so it was a drop-in replacement for npm start , still need to check on the npm build.

Excellent. This is exactly the conclusion I had come to about Parcel v1 & v2. Vite seems like the right alternative to me!

There was a bug in code due to which the error I reported was getting shown, so I fixed that. Another thing is, current code would run compile every time we changed compiler pass , which I changed to compile only when compile button is pressed. I observed that with original code, the page would hang up whenever I tried to select/de-select the passes.

This all sounds fine. If we ever want to get rid of the compile button and just have it automatically recompile, we can explore that separately.

After digging, I found that it originates from merge_namespace , which in turn originates from canonicalize_extern->canonicalize . As an experiment, I removed the canonicalize_extern call, and instead passed the original path directly to self.lib.add_extern(abs_path, exts); -> self.lib.add_extern(p.into(), exts); While this made the website work, I don't know if that is correct, or the code generated with it is correct or not

Got it; this makes sense. I think we probably want to keep the path canonicalization (which helps us avoid duplicating different imports of the same underlying file), but we can make that platform-dependent—that is, when compiling to a wasm target, we'll disable it, because paths are handled differently there.

If I'm correct in above; then as we don't need the import statements in the web (because we already resolve them beforehand) , would it be ok to do a target-based conditional compilation, where for wasm it will do p.into() and for other it will be as it is now?

Yes! This is exactly what I was imagining too.

Also, I personally use npm instead of yarn, so it made a npm lockfile instead of yarn lockfile. Do you have a specific preference for yarn, or is it ok to remove the yarn lockfile and add the npm one instead?

Nope; IMO switching to npm sounds great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Tracker Track various tasks
Projects
None yet
Development

No branches or pull requests

3 participants