Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman
, '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

Use donfig for V3 configuration - #1855

Merged
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config
May 10, 2024
Merged

Use donfig for V3 configuration#1855
jhamman merged 9 commits into
zarr-developers:v3from
maxrjones:v3-config

Conversation

@maxrjones

@maxrjonesmaxrjones commented May 8, 2024

Copy link
Copy Markdown
Member

Progress towards the proposal in #1772 to use Dask style config (donfig) for configuration in v3.

TODO:

  • Migrate sync configuration to donfig
  • Migrate runtime configuration to donfig
  • Add tests for setting configuration
  • Add docs for listing and setting configuration

@normanrznormanrz mentioned this pull request May 8, 2024
6 tasks
@jhammanjhamman added the V3 label May 8, 2024
@jhammanjhamman added this to the 3.0.0.alpha milestone May 8, 2024
@maxrjones

maxrjones commented May 9, 2024

Copy link
Copy Markdown
MemberAuthor

@jhamman fyi I've gotten as far as I can right now on the configuration and will be offline until wednesday. I started working on the byte order in 5de1db2, but while the tests pass I don't think it's fully correct which is why it's left out of this PR.

You're welcome to take over this PR or close if you think it'd be better to make progress separately. I'm free Wednesday 5/15 from 1-3 PM ET to work on this next if it's still an open issue at that time.

@jhammanjhamman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@normanrz - I picked up where @maxrjones left off on this one. Things are going well but I have one design question for you.

The question is what to do with the order config option. For now, I've added it to the ArraySpec object which works but, to some extent, mixes array metadata w/ array runtime options. The relevant constraints that I see are:

  1. We want to decide on an order at the time we create the AsyncArray object. From then on, all chunks and output arrays should use this. You'll notice a new attribute AsyncArray.order defined in the __init__ method. This is important because we don't wouldn't want the following to change the order:
    array=AsyncArray.open(...)
    config.set({'array.order': 'C'}}
    a1=array.getitem(...)
    config.set({'array.order': 'F'}}
    a2=array.getitem(...)
    asserta1.order==a2.order
  2. order does not make sense to include in the ArrayMetadata. It would be convenient to attach to that object but I don't think its the right choice -- we would have to special case serialization of that object so 👎 .

What do you think about the approach I've taken? Do you have other suggestions?

Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/array.py
Comment threadsrc/zarr/metadata.py

@normanrznormanrz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like it! I think ArraySpec is a good place to put it, because it is the specification of how an array shall be constructed. That makes sense to me!

Comment threadsrc/zarr/codecs/sharding.py Outdated
shape=chunks_per_shard + (2,),
dtype=np.dtype("<u8"),
fill_value=MAX_UINT_64,
order="C", # TODO!!!!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
order="C",# TODO!!!!
order="C",

A hard-coded C is fine here, because it is just the shard index. It doesn't surface into user code.

@jhammanjhamman changed the title WIP: Use donfig for V3 configurationUse donfig for V3 configurationMay 10, 2024
@pep8speaks

Copy link
Copy Markdown

Hello @maxrjones! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 624:101: E501 line too long (102 > 100 characters)

@jhamman
jhamman merged commit 5bb3375 into zarr-developers:v3May 10, 2024
@jhammanjhamman mentioned this pull request May 10, 2024
@maxrjones
maxrjones deleted the v3-config branch May 21, 2024 16:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants

@maxrjones@pep8speaks@normanrz@jhamman