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 Ubuntu install script #8988

Closed

Conversation

majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Mar 7, 2024

Ubuntu script fails when you run it twice.
Add missing SUDO to other system commands in the helper function.
Check if conda is already present

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 7, 2024
Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit ec28cda
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/65f385b8845d86000854bcab

@majetideepak majetideepak changed the title Fix installing ubuntu dependencies Fix installing ubuntu dependencies in workflows Mar 7, 2024
@majetideepak majetideepak changed the title Fix installing ubuntu dependencies in workflows Fix installing ubuntu dependencies in benchmarks workflow Mar 7, 2024
@assignUser
Copy link
Collaborator

This was intentional to only install apt deps and use resolve_dependency for the rest, what is missing?

@majetideepak
Copy link
Collaborator Author

This was intentional to only install apt deps and use resolve_dependency for the rest, what is missing?

@assignUser thanks for the clarification. I will undo the workflow change.
Was this intent for all workflow files? We had to remove sourcing recently for the nightly jobs https://github.com/facebookincubator/velox/pull/8943/files

@assignUser
Copy link
Collaborator

@majetideepak Yes, my intention was to build the deps within the velox build so they can be cached by ccache (which we would need to manually setup for the projects otherwise). The idea being that the ccache would then speed up the overall time compared to building the deps stand alone. But with actions/cache being broken for build caching it doesn't matter for the moment.

But I would like to preserve a way to only install the apt/yum deps when using the setup scripts and (like you added in the other PR) a way to only install specific deps instead of all of them.

@majetideepak majetideepak changed the title Fix installing ubuntu dependencies in benchmarks workflow Fix Ubuntu install script Mar 14, 2024
@majetideepak majetideepak marked this pull request as ready for review March 14, 2024 23:20
Copy link
Contributor

@kgpai kgpai left a comment

Choose a reason for hiding this comment

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

Thanks, I will start the merge when the scheduled jobs pass.

@majetideepak
Copy link
Collaborator Author

majetideepak commented Mar 15, 2024

Looks like the scheduled Fuzzer job is not testing this change which I thought it would.

@assignUser
Copy link
Collaborator

assignUser commented Mar 15, 2024

Looks like the scheduled Fuzzer job is not testing this change which I thought it would.

it's because it always checks out main, I fixed that in https://github.com/facebookincubator/velox/pull/8734/files#diff-f4109e225f86cb2508cacc3bd71f0782d0d42dd5caa207350bc8f72abbb18af3R82

(I also fixed the need for running the whole script vs sourcing it as well, but for local use this is of course still needed)

@majetideepak
Copy link
Collaborator Author

@kgpai can we merge this PR so that we can use it in #8734
Thanks!

@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in 84f94bc.

assignUser added a commit to assignUser/velox that referenced this pull request Mar 18, 2024
assignUser added a commit to assignUser/velox that referenced this pull request Mar 18, 2024
@majetideepak majetideepak deleted the fix-workflow-files branch March 20, 2024 20:50
Joe-Abraham pushed a commit to Joe-Abraham/velox that referenced this pull request Jun 7, 2024
Summary:
Ubuntu script fails when you run it twice.
Add missing SUDO to other system commands in the helper function.
Check if conda is already present

Pull Request resolved: facebookincubator#8988

Reviewed By: bikramSingh91

Differential Revision: D54955154

Pulled By: kgpai

fbshipit-source-id: 946c724ef51f5ae0e8527e8e914f6eb326088746
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants