Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Feature: Batch Insights support #336

Merged
merged 6 commits into from Feb 5, 2019
Merged

Feature: Batch Insights support #336

merged 6 commits into from Feb 5, 2019

Conversation

brnleehng
Copy link
Collaborator

Copy link

@bgklein bgklein left a comment

Choose a reason for hiding this comment

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

Just a couple clarifying questions and a suggestion to use batch explorers .io landing page

@@ -235,8 +235,15 @@ BatchUtilities <- R6::R6Class(
commands <- c(commands, args$commandLine)
}

commands <- linuxWrapCommands(commands)
if (!is.null(args$applicationInsights)) {
commands <- gsub("wait",
Copy link

Choose a reason for hiding this comment

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

Is wait something you add to the user cmd?

Copy link

Choose a reason for hiding this comment

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

I'm assuming so

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes we use wait for the following user cmd.

Note: Github has a rate limit of up to 60 requests per hour. Unauthenticated requests are associated with the originating IP address. [Link](https://developer.github.com/v3/#rate-limiting)

For more information about these tools,
- [Batch Explorer](https://github.com/azure/batchexplorer)
Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will replace it.

# Usage:
# setup_node.sh

apt-get -y install linux-image-extra-$(uname -r) linux-image-extra-virtual
Copy link

Choose a reason for hiding this comment

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

Reason for this being removed? Just unneeded now? I don't see it moved to code anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unneeded code, once we moved to using Azure Batch's container image. This cluster setup is not needed.

@brnleehng brnleehng merged commit 31eec56 into master Feb 5, 2019
@sorenvind
Copy link
Contributor

@brnleehng The merge of this PR is the root cause of the bug I attempted to fix in #340. However, the bug fix did not succeed.

By merging the present PR to master, the file at https://raw.githubusercontent.com/Azure/doAzureParallel/master/inst/startup/cluster_setup.sh was changed. The file is referenced in old versions of doAzureParallel (see e.g. https://github.com/Azure/doAzureParallel/blob/v0.6.3/R/cluster.R#L145). Since the set up of docker was removed by this PR, all pools/jobs orchestrated using previous versions of doAzureParallel will fail until this PR is reverted or docker is installed again by the cluster_setup.sh file. It is unclear to me if the version on master could potentially work.

This change has broken the daily runs in our production system relying on Batch for two days now. What can I do to help fix this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants