_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

_sandboxremote.py: avoid reusing failed actions - #2033

Draft
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions
Draft

_sandboxremote.py: avoid reusing failed actions#2033
abderrahim wants to merge 1 commit into
masterfrom
abderrahim/failed-actions

Conversation

@abderrahim

Copy link
Copy Markdown
Contributor

This is a proposal towards #2020. It's probably not the most efficient way to do it, but at least it works.

What this does is:

  • ignore failures found in the action cache
  • ask the remote execution service to not look in the cache

stub = self.exec_remote.exec_service
request = remote_execution_pb2.ExecuteRequest(
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=False
instance_name=self.exec_remote.instance_name, action_digest=action_digest, skip_cache_lookup=True

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't want to always skip cache lookup. If no action-service-cache is declared, internal action cache lookup by the remote execution server should still be used.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, even when the action-cache-service is defined, there is still value in having the execution re-do cache lookup (in case the action is requested and built by someone else before our action reaches the top of the queue).

You're right that this is probably working around a "broken" remote execution server. I've only tested it with buildbox-casd, and wasn't aware it was different with other servers.

@juergbi

Copy link
Copy Markdown
Contributor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

@abderrahim

Copy link
Copy Markdown
ContributorAuthor

I think the main issue is that failed actions are cached in the action cache in the first place. While there may be circumstances where caching failed actions is useful, I don't think we need this for BuildStream at all (given that we have a higher level caching mechanism where we already support caching failures) and most remote execution servers default to not caching failed actions, mainly because failures may be spurious, e.g., due to a worker running out of RAM.

BuildGrid caches failures by default but it can be disabled with cache-failed-actions: false. buildbox-casd currently unconditionally caches failures but I think we should change that or at least add an option to disable it.

Yeah, I was using this with buildbox-casd as a server. The failures in question were due to a bug in buildbox-fuse.

On the BuildStream side, might it be sufficient to skip action cache lookup (direct action cache query as well as indirectly via Execute()) if context.build_retry_failed is set?

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

@juergbi

Copy link
Copy Markdown
Contributor

The thing is context.build_retry_failed is only set when passing the option on the command line or the config file. It doesn't work when you choose retry on the prompt.

Good point but maybe we can find a way to forward this information also in the interactive retry case.

Sign up for freeto 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

@abderrahim@juergbi