Skip to content

Fix command errors in Nebari server run for templates #317

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

Merged
merged 8 commits into from
Sep 11, 2023

Conversation

guptaaryan16
Copy link
Contributor

Description

This PR is a follow up PR for the fixes in the code execution while testing in the Nebari server. Particularly we faced two issues, first if we open a template in Nebari, its default folder will be root and if it is the second template opened, then it will try to download and overwrite the previous one. This can be fixed by fixing the nbuid-> nbuid + '-nebari'.

Also we add some extra commands that can help in creating a new directory and store templates files in that specific directory.

Fix #314

Additional context

This issue was discussed in Discord this week.

What is the purpose of this pull request?

  • Bug fix
  • New feature
  • Other

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for code-generator ready!

Name Link
🔨 Latest commit c66e3c1
🔍 Latest deploy log https://app.netlify.com/sites/code-generator/deploys/64feeab6ee1d13000835907e
😎 Deploy Preview https://deploy-preview-317--code-generator.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@guptaaryan16
Copy link
Contributor Author

Hey @vfdev-5 seems to work as expected now, can you check on your server as well once?

@vfdev-5
Copy link
Member

vfdev-5 commented Sep 11, 2023

Screen Shot 2023-09-11 at 09 10 50

Is it expected to have this ? How did you test it ?

@guptaaryan16 this does not work!
image

@guptaaryan16
Copy link
Contributor Author

Screen Shot 2023-09-11 at 09 10 50 Is it expected to have this ? How did you test it ?

@guptaaryan16 this does not work! image

Actually for other configurations like text-segmentation, it seems to work, I guess we need to delete some previous notebooks generated for nebari.js like the one you mentioned

@vfdev-5
Copy link
Member

vfdev-5 commented Sep 11, 2023

Actually for other configurations like text-segmentation, it seems to work, I guess we need to delete some previous notebooks generated for nebari.js like the one you mentioned

It's very annoying that I have to discover during the review. You could have tested the first template at least

@guptaaryan16
Copy link
Contributor Author

It's very annoying that I have to discover during the review. You could have tested the first template at least

Actually I was testing some solutions for this problem, and it seems like updating nbuid-> nbuid + '-nebari' + <last_commit_hash> may work. Let me test this and then we can merge this PR.

@vfdev-5
Copy link
Member

vfdev-5 commented Sep 11, 2023

Actually I was testing some solutions for this problem, and it seems like updating nbuid-> nbuid + '-nebari' + <last_commit_hash> may work. Let me test this and then we can merge this PR

Commit suffix is not related to nebari option but have to be considered in general. But in general appending a commit is not a good solution as we would generate new uuid everytime someone pushes to main (even unrelated change).

We may want distinguish cases when the app is run from netlify PR deployement vs from code-generator.pytorch-ignite.ai/ and append commit from PR only, IMO

@guptaaryan16
Copy link
Contributor Author

Commit suffix is not related to nebari option but have to be considered in general. But in general appending a commit is not a good solution as we would generate new uuid everytime someone pushes to main (even unrelated change).

We may want distinguish cases when the app is run from netlify PR deployement vs from code-generator.pytorch-ignite.ai/ and append commit from PR only, IMO

Let me revert the changes for now and make a second PR for this deployment vs PR build case

Copy link
Member

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @guptaaryan16

@vfdev-5 vfdev-5 merged commit e273a27 into pytorch-ignite:main Sep 11, 2023
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