distro: add OutputFilename option to ImageOptions

This commit allows to change the filename when creating a manifest from an image type. This is useful when doing a UI for users, e.g. this will allow us to support:
$ image-builder build rhel-9 type:qcow2 --output foo.img

(this will also be useful for bootc-image-builder).
Well, osbuild itself will still put it under a directory when doing main_cli.py:exports() but that is something orthogonal.
I couldn't find a good way to test this end-to-end though (without doing a full manifest with depsolve) because the various DiskImage(), EdgeCommitImage() functions are assigned to ImageType.method so iterating over the image types will only give me the ability to run Manifest() which creates a manifest.Manifest but from there I cannot access any pipelines without producing a full osbuild manifest. Ideas welcome, I did test it via the "image-builder-cli" branch. Maybe I'm missing something but this area of the code might need some tweaks to make it more testable.
P.S. I also explored an alternative approach that would allow to set the filename on the image type in (in https://github.com/osbuild/images/compare/main...mvo5:make-target-filename-setable?expand=1) but this one here feels slightly cleaner.

My suggestion is to instead move internal/target pkg from osbuild-composer to images and reuse what is there. Please take a look at https://github.com/osbuild/osbuild-composer/blob/d1bf0a77f046c909f6fc99783310b645c37b312f/internal/target/target.go#L23-L35

My suggestion is to instead move internal/target pkg from osbuild-composer to images and reuse what is there. Please take a look at https://github.com/osbuild/osbuild-composer/blob/d1bf0a77f046c909f6fc99783310b645c37b312f/internal/target/target.go#L23-L35
EDIT: Probably not reuse as is, but I would hate us having the same thing defined in multiple places and multiple structures. Maybe some parts can be extracted / refactored or reused. What I didn't realize at the moment of making this initial comment was that the Target structure contains status-related items, which we probably don't need in images.

Thanks for the suggestion - maybe I'm misunderstanding and/or I need to look more at this but it seems to me that the "target.Target" is a higher level concept on a different layer that is applied after the manifest creation (c.f. https://github.com/osbuild/osbuild-composer/blob/main/internal/cloudapi/v2/server.go#L163) and is more an "after-fact" concept (i.e. do something to the image after it was created with the fixed filename).  I'm looking for a way to already encode the target filename in the generated manifest.
It seems to me to me this and the target.ImageName are (mostly) orthogonal, i.e. this is strictly about the local filename that is encoded in the generated manifest. I also looked at the options in target.Target and it seems only the "ImageName" (OutputFilename in this PR) is useful, the others can be derived (like "Created") or have no meaning in the "generate-a-manifest" use-case (like "Uuid"). But maybe I'm missing something, happy to have a chat about how you envision this.

I couldn't find a good way to test this end-to-end though

Something for the gitlab pipelines maybe?

TL;DR; my biggest problem with this approach is that ImageOptions.OutputFilename and Target.ImageName can be used to achieve the same thing - produce an artifact with a given filename. One approach will be used in the image-builder-cli, and another is currently used by build-composer. While both structures will be available and used in osbuild-composer, making the code potentially confusing.

Thanks for the suggestion - maybe I'm misunderstanding and/or I need to look more at this but it seems to me that the "target.Target" is a higher level concept on a different layer that is applied after the manifest creation (c.f. https://github.com/osbuild/osbuild-composer/blob/main/internal/cloudapi/v2/server.go#L163) and is more an "after-fact" concept (i.e. do something to the image after it was created with the fixed filename). I'm looking for a way to already encode the target filename in the generated manifest.

While your point of view is definitely correct, I'm afraid both concepts will eventually be used to solve the same problem—generating an artifact with the user-provided name. The fact that one is a higher-level concept and the other is a lower-level concept becomes just an implementation detail hidden from the user.
On a conceptual level, I don't think ImageOptions is the correct place to encode this type of information. One of the reasons is that it would be functionally almost an identical type of information to the one encoded by Target. In addition, one can export multiple osbuild pipelines as part of a manifest build, and each of these exported pipelines may produce one or more files as an artifact. Being able to export multiple pipelines as part of a single manifest build is something we should keep as a possibility, at least internally. Target conceptually represents exporting a single osbuild pipeline (also nicely represented by OsbuildArtifact).

It seems to me to me this and the target.ImageName are (mostly) orthogonal, i.e. this is strictly about the local filename that is encoded in the generated manifest. I also looked at the options in target.Target and it seems only the "ImageName" (OutputFilename in this PR) is useful, the others can be derived (like "Created") or have no meaning in the "generate-a-manifest" use-case (like "Uuid"). But maybe I'm missing something, happy to have a chat about how you envision this.

I don't think we should modify manifests to generate files with the desired filename. If we do, we'll need to make this possible with any tool that generates the manifest (OTK or anything else). Instead, image definitions usually encode metadata about what each pipeline can produce if exported. Renaming the produced artifact afterward seems like a cleaner separation of functionality.
I admit that your proposed change is simple, clean, and an easy way to achieve the functionality that you need in the image-builder-cli. However, I'm afraid that it will add friction and complicate future and more complex use cases, such as potentially exporting multiple artifacts (multiple image files, SBOMs, etc.).
FWIW, I don't think Target is usable as it is implemented in osbuild-composer. But it is suitable as a concept and inspiration. We should probably create an extensible representation for user preferences when exporting any artifact (an image file, a manifest, or anything else). As you've mentioned, we may need something different when generating just the manifest, something different when producing an image file, and something different when importing an image to the target environment and naming it there. Having a single OutputFilename field in the ImageOptions won't solve all of these use cases either.
I may be overthinking this a bit, but I'm sure that we will find some middle ground.

I may be overthinking this a bit, but I'm sure that we will find some middle ground.

I get where you're coming from (I think) and I don't disagree.  The way I see it (and please tell me if I'm going a completely different direction from what you were thinking), we have two major categories of options: Reusable and one-off (or build-specific) options.
Reusable options are the ones we put in blueprints.  Package selection, users, kernel args, enabling services, etc, are valid for all sorts of image types and we generally consider that anything that goes in a blueprint can be reused for multiple builds across time, image types, distributions, even architectures.
One-off options are things that are specific to a single build.  The target image type, upload target, ostree commit source, ostree ref.  Things like these originally came from the composer-cli command line arguments and later as extra properties in the build request in the API.
This distinction was never really formalised (I don't think it was anyway) and we generally had an understanding that some things don't belong in a blueprint, so we'd make it a build option, or target option, or something similar.  Maybe we should think about this distinction in these (or similar) terms and reflect it in the code as well.  Also, perhaps we need a third category here: Options that have no effect on manifest generation (like the upload target), which would be options that are never passed to Manifest(), but still have some effect on the output after the build.

I don't think we should modify manifests to generate files with the desired filename. If we do, we'll need to make this possible with any tool that generates the manifest (OTK or anything else). Instead, image definitions usually encode metadata about what each pipeline can produce if exported. Renaming the produced artifact afterward seems like a cleaner separation of functionality.

I don't fully agree with this, though I see your point.  I don't think it's a bad idea to modify a manifest to output a desired filename.  That said, allow me to argue with myself about the two possible ways of thinking about this:

The filename generated by a pipeline (especially the exported pipeline) is useful for a tool to know so that it can manage the output once it's done.  Since it's useful to know it, it might also be useful to be able to define it, making it simpler to manage.  Instead of reading some metadata from the build or the manifest generator, it tells the manifest generator what it wants, the same way it tells it where the output directory should be.  This is what this PR is doing.
The filename generated by a pipeline is an internal implementation detail.  It's parameterised in the code, but it only matters that it's consistent between pipelines and it doesn't matter what the specific filename is and should never be seen by a user of a tool built around a manifest generator for osbuild.  Manifest generators know the filename created by each pipeline because they set it themselves as part of the pipeline creation (when it's a single file and not a tree) and so they can expose an option to set a filename, but what they actually do is basically just rename the file before returning it.

No. 2 is basically what osbuild-composer with composer-cli do today.  composer-cli compose image --filename images/my-fancy-new-image.qcow2 <UUID> simply copies a file to a user-defined path.
Relating this back to the option categories I was describing above, this means that the output filename is part of the third category: it's specific to a single build and has no effect on the manifest itself, it only defines what the build tool does after the image is finished building.

FWIW, I don't think Target is usable as it is implemented in osbuild-composer. But it is suitable as a concept and inspiration. We should probably create an extensible representation for user preferences when exporting any artifact (an image file, a manifest, or anything else). As you've mentioned, we may need something different when generating just the manifest, something different when producing an image file, and something different when importing an image to the target environment and naming it there. Having a single OutputFilename field in the ImageOptions won't solve all of these use cases either.

I think I'm coming around to the same conclusion.  I certainly agree that "Target is [not] usable as it is implemented in osbuild-composer".  But we should definitely think about this more generally and design something that will be useful for all the use cases we can expect going forward.


Add change detection for record editing

Similar to resource editing change detection, also add it for record creation/edit
This is referring to the "Unsaved changes warning"

Related issue: https://github.com/cortezaproject/corteza/issues/1478


Get correct field length in ngrest config from database field

If I want to reference a hex color (varchar(6) in mysql database) - I can't configure the correct field length in ngrest config and have to use ->text() without proper length check.

general ngrest thread #417


Overwriting files does not work

Reported in, https://github.com/owncloud/ocis/issues/45
can confirm locally
Access to XMLHttpRequest at 'http://localhost:20080/remote.php/webdav/20191007_105648.jpg' from origin 'http://localhost:8300' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.
files.bundle.js:7 PUT http://localhost:20080/remote.php/webdav/20191007_105648.jpg net::ERR_FAILED

fixed by https://github.com/cs3org/reva/pull/370/commits/15e0d3f5b27c2f043e2ab02a589605ebd6f176b9

fixed by https://github.com/cs3org/reva/pull/370/files#diff-af7641b220981605161a6ea06b79d3a7R95


Custom Background Settings: mention comment syntax

In "Custom Background Settings" we see

Enter a tile URL template below.
Supported WMS tokens:
{proj}: requested projection (EPSG:3857 only)
{wkid}: same as proj, but withou...

All that is great, but let's say we have two frequently used favorite URL templates.
Well, if we could at least know how to comment:
<!-- TEMPLATE1 -->
TEMPLATE2

Then we wouldn't have to type the whole thing in again when switching.
(Sure, if you would help us keep track of our favorite templates separately, that would even be better. But still, a comment syntax should be documented.)

@jidanni You already opened #7426 which is about support for multiple custom backgrounds. I'd say let's focus on that solution rather than spending time on an advanced workaround. You can always just keep your endpoints in a notes files separate from iD and copy-and-paste them as needed.

OK, but,

There is plenty of room in that box for comments:

Nobody gets it right at the first attempt,
so there should always be room for comments:

# https://{switch:a,b,c}.mmmr.openstreetmap.org/{zoom}/{x}/{y}.png
# https://{switch:a,b,c}.pppz.openstreetmap.org/{zoom}/{x}/{y}.png
https://{switch:a,b,c}.tileqq.openstreetmap.org/{zoom}/{x}/{y}.png

without having to resort to Notepad etc.

If someone wants to open a PR for this then that sounds okay. But it takes real work to build and maintain features and unfortunately we don't have the resources to do something just because there's space for it.

(OK, but pssst... tell me the comment syntax. I mean there is already fancy preprocessing  of those URLs, so I bet you guys didn't forget to add one... Or just add {ignore: ..............} .)

@jidanni There is no comment syntax. You paste in a URL, we replace the tokens with the parameters, and that's it. "URL comments" aren't really a thing.

Every 'language' needs them,
https://stackoverflow.com/questions/2524367/inline-comments-for-bash
Anyway with {comment: ..............} syntax, iD could have them too in a jiffy!

@jidanni I thought this went without saying but it's just a URL input box, it's not a bash script.

This thread got me thinking: One could miss-use a hash-syntax to kind off comment out parts of an URL. I don't think its really useful, but it does work :-).
Format:
URL1#URL2
And to switch the URL, I can manually copy-paste the URL to URL2#URL1.
Example:
Input for the custom map dialogue:
https://isk.geobasis-bb.de/ows/dop20c_wms?FORMAT=image/png&TRANSPARENT=TRUE&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=bebb_dop20c&STYLES=&SRS={proj}&WIDTH={width}&HEIGHT={height}&BBOX={bbox}#https://isk.geobasis-bb.de/mapproxy/dgm/service/wms?FORMAT=image/jpeg&TRANSPARENT=TRUE&VERSION=1.1.1&SERVICE=WMS&REQUEST=GetMap&LAYERS=dgm&STYLES=&SRS={proj}&WIDTH={width}&HEIGHT={height}&BBOX={bbox}
As an URL:
https://www.openstreetmap.org/edit#background=custom:https://isk.geobasis-bb.de/mapproxy/dgm/service/wms%3FFORMAT%3Dimage/jpeg%26TRANSPARENT%3DTRUE%26VERSION%3D1.1.1%26SERVICE%3DWMS%26REQUEST%3DGetMap%26LAYERS%3Ddgm%26STYLES%3D%26SRS%3D{proj}%26WIDTH%3D{width}%26HEIGHT%3D{height}%26BBOX%3D{bbox}%23https://isk.geobasis-bb.de/ows/dop20c_wms%3FFORMAT%3Dimage/png%26TRANSPARENT%3DTRUE%26VERSION%3D1.1.1%26SERVICE%3DWMS%26REQUEST%3DGetMap%26LAYERS%3Dbebb_dop20c%26STYLES%3D%26SRS%3D{proj}%26WIDTH%3D{width}%26HEIGHT%3D{height}%26BBOX%3D{bbox}&disable_features=boundaries&map=18.60/52.41051/12.54939

See @quincylvania, without a documented comment syntax, users will hack up their own, doubling the size of each request ☹☺


Content-Length is removed by OkHttp but getContentLength returns -1

The web server does return a positive "Content-Length" header (as checked in debug mode and also separated test) but OkHttp removed it so the header cannot be retrieved by getHeaderFields. I guess the removal is intended as there is a getContentLength method to retrieve the content length. However, in my case, the content length is -1.
The web server supports gzip/inflat, and the problem occurs when "Accept-Encoding" is not set (also when "Accept-Encoding" is set to "identity", as i tried after seeing #116). When "Accept-Encoding" is set to  "gzip,deflate", content length is normal/positive. So i suppose the problem is caused by a transparent/automatic gzip is happening.
I suppose I shouldn't set "Accept-Encoding" to gzip, because when I connect to a server that doesn't support zip, my code can't tell whether the input stream is gzip'd or not, right?
btw, I need to get the content length either from the API or from HTTP Header for change detection.
The following issues are similar but apparently different issue.
https://github.com/square/okhttp/issues/116
https://github.com/square/okhttp/pull/250

@swankjesse:
OkHttp only strips the content length if it's doing transparent gzip

Do you mind explaining why?

There's two lengths:

O: the length of the original content
C: the length of the compressed content

When we strip the content length from the HTTP response, it's because we know C but the application developer is expecting O. (And will receive O bytes when they consume the entire stream).

@swankjesse  we want content length over the wire.  I didn't find a way to unzip the response manually to support not getting the content length header  stripped.

There's two lengths:
o: the length of the original content
c: the length of the compressed content

We want c for other reasons.  I think I was able to get it by reading the Content-Length header inside of a NetworkInterceptor.  Diminishing returns in our case, so I hope it's correct.  We wanted c but o will tell us almost as much.

Yep. Reading it in a network interceptor is the way to go. Alternately you can do gzip yourself; that’s just another interceptor to make.

@samtstern
Hi Sam, just bringing this to your attention, due to this behaviour of OkHttp, if anyone is relying on OkHttp's transparent gzip decompression, Firebase Performance monitoring fails to report Payload size (Content-Length gets omitted)

@arnavzoman thanks for the heads up! I don't completely understand the issue here (I don't work on Firebase Perf directly), would you mind filing an issue on firebase-android-sdk explaining what's going wrong so we can assign it to the right people?

Cool, I have done that
https://github.com/firebase/firebase-android-sdk/issues/952

2. No. The original content length is discarded.

swankjesse@ Thanks for explanation. I have a couple of questions:
Q1: Do we discard original content-length because we only know about compressed content (which is not what the caller is expecting) at that point?
Q2: Does reading from the NetworkInterceptor provides Original content length of Compressed content length?
Q2.1: If Original, then is it possible for OkHttp to provide the original content length by adding the Network Interceptor themselves rather the caller implementing it?
Q2.2: If Compressed, then is there any other way caller can get the Original content length? or the caller has to read the entire stream to get that answer?

@swankjesse Would be great if you can check out https://github.com/square/okhttp/issues/259#issuecomment-549639151 once.

Note that the original network response headers are available:
String contentLengthHeader = response.networkResponse().header("Content-Length")

Thanks swankjesse@
Is there also a way to get the compressed content length info (so basically the actual wire download size)?

You could get it from the EventListener.
https://square.github.io/okhttp/events/
Or create a Source that counts.
  /** A source that keeps track of how many bytes it's consumed. */
  private class CountingSource(source: Source) : ForwardingSource(source) {
    var bytesRead = 0L

    override fun read(sink: Buffer, byteCount: Long): Long {
      val result = delegate.read(sink, byteCount)
      if (result == -1L) return -1L
      bytesRead += result
      return result
    }
  }

Reading the stream of data may be the last thing we would do. But thanks for highlighting that well.
Can you provide example with EventListener?

Install an EventListener, and override this method:
https://square.github.io/okhttp/4.x/okhttp/okhttp3/-event-listener/response-body-end/


Non-English attachment don't get right download file name.

For example, if I upload a file named "檔案.pptx", download this attachment I don't get the same file name. This is because file name encoding and browser issues. Related discussion:
http://stackoverflow.com/questions/93551/how-to-encode-the-filename-parameter-of-content-disposition-header-in-http

this looks good, could you create some tests for this?
could you create a test exposing the bug? meaning a test that will fail without your patch


Download only the mp3 version of videos.

There are many music creators on Instagram. Some of them do not post their covers to other platforms. Using this feature users will be able to download just the audio version of the songs. Additional functionality can be added as after the song is downloaded, we can use audio recognition api like shazam to find the song and create a playlist in platforms like spotify or youtube music.
How could the user invoke the new function? Which problem would it solve?
By using --create playlist flag to create a playlist or --audio-only flag to listen only to audio.
If new information is obtained, how would it be further processed?
No new information is required for this. This library already has the option to download videos. We only need to keep the audio part of that.
Describe the solution you'd like
There are many music creators on Instagram. Some of them do not post their covers to other platforms. Using this feature users will be able to download just the audio version of the songs. Additional functionality can be added as after the song is downloaded, we can use audio recognition api like shazam to find the song and create a playlist in platforms like spotify or youtube music.
Describe alternatives you've considered
Videos can be listened as audio but the playlist creation feature is not possible.
How do you achieve the here-described goal without the feature you are requesting?
If I want to listen to the reels by a specific creator as audio there is no tool to do that.
If the feature request is accepted, would you be willing to submit a pull request?
Yes

Additional context
Implementation and scope of this feature can be discussed further and any input on the same would be appreciated.

This is out of scope, Instaloader should not postprocess the downloaded content or invoke further APIs. The downloaded data can be easily processed in external tools.


⚠ Bofrost.lu has degraded performance

In d75c3ce, Bofrost.lu (https://www.bofrost.lu) experienced degraded performance:

HTTP code: 200
Response time: 2279 ms

Resolved: Bofrost.lu performance has improved in 31b3dde after 15 minutes.


Update virtual-machines-windows-winrm.md

Fixed typos.

Hi @alexandair, I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.
TTYL, AZPRBOT;

@alexandair : Thanks for your contribution to the Azure documentation! The author, @singhkay, has been notified to review your proposed change.

lgtm @alexandair Thanks for the fix!

@singhkay - Kay, this PR has a merge conflict. Please resolve the merge conflict - we have instructions on the wiki.

Thanks for fixing, Kay. @alexandair - Your updates were pulled over to the private, they should be published later today.


CPP: Speed up OverflowStatic.ql

Three separate changes designed to speed up 'OverflowStatic.ql' - though in practice a lot of this stuff is cached so it will speed up the first query to touch it.

So it turns out the third commit contained a gross logic error (which didn't normally show up in the results of the 'OverflowStatic.ql' query), which was naturally responsible for the speedup on that one.  So I've dropped it.

I've just re-triggered Language-Tests/CPP


Optimize Reachability_CanReach_Patch

When a large raid is coming towards the base, calls to this patch reach noticeable timings (0.172ms in my testings). It should be possible to reduce the number of variables that this patch needs to traverse to reach the pathGrid, and there might be some other improvement opportunities as well.

Impact reduced during refactor.


CountedNMap implementation

add CountedStorageNMap implementation which  is similar as CountedStorageMap for StorageMap but for the StorageNMap
https://github.com/paritytech/substrate/issues/10602

@bkchr , @shawntabrizi don't want to be tactless, but can we proceed with this PR, it will be helpful to solve this issue https://github.com/paritytech/substrate/issues/9060

This PR seems valuable, yet hat slipped through the crack. Partially due to lack of labeling and project assignment from our side. Hopefully will get some review soon. Thanks @Mr-Leshiy!

I'm honestly interested to know why can't we just have a CountedStorageNMap instead? The CountedStorageDoubleMap can just be a type alias to a CountedStorageNMap that way (and the same can also be said for CountedStorageMap).

@KiChjang , so sorry for the delay.

I'm honestly interested to know why can't we just have a CountedStorageNMap instead? The CountedStorageDoubleMap can just be a type alias to a CountedStorageNMap that way (and the same can also be said for CountedStorageMap).

For my understanding it is impossible to implement in such way, because all these types of storages have different type of interfaces and also have different type of implementation. So in this case it is impossible to unify them and hide under the same implementation and interface.

I'm sorry but this doesn't expose what exactly the impossibility is. Why is it not as simple as this:
type CountedStorageDoubleMap<P, H1, K1, H2, K2, V, Q, O, M> = CountedStorageNMap<P, (NMapKey<H1, K1>, NMapKey<H2, K2>), V, Q, O, M>;

What exactly is different about the interfaces?

@KiChjang , as a reference we can consider the following function from the original double map and from the original nmap  contains_key.
DoubleMap function signature contains_key:
contains_key<KArg1, KArg2>(k1: KArg1, k2: KArg2) -> bool

Nmap function signature contains_key:
contains_key<KArg: EncodeLikeTuple<Key::KArg> + TupleToEncodedIter>(key: KArg) -> bool

Okay, then we don't do a type alias, but call into the NMap methods underneath, i.e.
fn contains_key<KArg1, KArg2>(k1: KArg1, k2: KArg2) -> bool {
    NMap::contains_key(((k1, Hasher1::hasher()), (k2, Hasher2::hasher())))
}

The point here is to reduce the amount of code that we have to maintain by using NMap behind the scenes.

@KiChjang , But how it will helps if I will replace an underneath implementation from the native DoubleMap storage to the NMap ? And if so I am coming with the general question why we have to different implementations of DoubleMap and NMap ?

@KiChjang I gotcha, sorry that was so annoying with questions and doubts, but I was not totally understood what do you mean and why need to change it 😊
So, what do you think if I will change this implementation just for the NMap ? And later will figure out how to extend it to be compatible with the DoubleMap interface ?

That sounds like a workable plan to me. It may also be easier if you attempt to convert the existing StorageDoubleMap to use StorageNMap underneath first, but having a CountedNMap implementation right now is also valuable as well.

In progress...

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1753993

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1753994

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1753995

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1755190

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1755320

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - test-linux-stable
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1755319

Hmm... since we still have StorageDoubleMap nowadays, I think we do have to add a CountedStorageDoubleMap after all, since that is what people would expect, but we can then have it actually use a CountedStorageNMap underneath.

@KiChjang , totally agree with you. But I would prefer to make StorageDoubleMap in the separate PR. But if you think that ti should be done here, I can do it, but will need some time for it :)

@KiChjang , totally agree with you. But I would prefer to make StorageDoubleMap in the separate PR. But if you think that ti should be done here, I can do it, but will need some time for it :)

Totally fine as well. That also reminds me -- CountedStorageMap itself can use StorageNMap underneath too, no reason to have separate implementations of it.

The CI pipeline was cancelled due to failure one of the required jobs.
The job name - cargo-check-benches
The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1980352

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2060150

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2060151

Still in progress ...

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2206752

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2206825

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2206826

@KiChjang I have updated and seems fixed with your suggestion. I have tried your solution but unfortunately it does not work.
I have implemented it as follows.
impl<'a, T: EncodeLike<U> + EncodeLikeTuple<U>, U: Encode> EncodeLikeTuple<U>
	for codec::Ref<'a, T, U>
{
}
impl<'a, T: EncodeLike<U> + EncodeLikeTuple<U>, U: Encode> crate::storage::private::Sealed
	for codec::Ref<'a, T, U>
{
}

I have tried different approaches but seems it would be the best one. What do you think ?

Made a small relocation change, but otherwise everything LGTM.

Before this can be merged we'll likely also need to add storage_alias macro support for this map along with some tests, similar to how it's done here: https://github.com/paritytech/substrate/pull/13366
Either that or a new issue, though it might be a nice precedent for posterity to have it all in one PR.

Still in progress

Still in progress

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate-macos
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2886734

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2886717

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2886718

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2886732

@Mr-Leshiy is there any progress on this?

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3018311

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3018300

for convenience and boilerplate when declaring a storage in a pallet we usually declare like this:
	#[pallet::storage]
	pub(super) type Dummy2<T: Config> = CountedStorageNMap<_, T::Balance>;

And the macro will create the type to replace the first type parameter _.
So this need to be handle in the pallet macro to be usable in a pallet.

Should this also contain the counter_storage_final_key and map_storage_final_prefix methods to keep it consistent with CountedStorageMap?

And the macro will create the type to replace the first type parameter _.
So this need to be handle in the pallet macro to be usable in a pallet.

@thiolliere does it requires any changes in this PR ?
If so, I am correct that it is needed to modify #[pallet::storage] macro to handle desired behavior ?

@aaronbassett not sure, because I have found only one usage of these methods for the CountedMap
https://github.com/Mr-Leshiy/substrate/blob/687ddfadfe61985ab94c852ac99c0b4794ca8391/frame/bags-list/remote-tests/src/snapshot.rs#L25 .
But maybe someone else can elaborate on this one

And the macro will create the type to replace the first type parameter _.
So this need to be handle in the pallet macro to be usable in a pallet.

@thiolliere does it requires any changes in this PR ? If so, I am correct that it is needed to modify #[pallet::storage] macro to handle desired behavior ?

I think it is better to put in the same PR as types those are specifically designed for usage in pallets.
also macro does allow to give named type parameter, maybe only those 2 files need to be touched:
frame/support/procedural/src/pallet/parse/storage.rs
frame/support/procedural/src/pallet/expand/storage.rs

@aaronbassett not sure, because I have found only one usage of these methods for the CountedMap https://github.com/Mr-Leshiy/substrate/blob/687ddfadfe61985ab94c852ac99c0b4794ca8391/frame/bags-list/remote-tests/src/snapshot.rs#L25 .
But maybe someone else can elaborate on this one

it seems those kind of method are sometime used in tests, it can be useful to ignore some storage cost in benchmarking also.

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3039116

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3039131

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3041888

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3041874

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3041873

@thiolliere done !
But I don't know how it is possible to test this #[pallet::storage] macro :( unfortunately have not found any related examples

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3045793

@thiolliere done !
But I don't know how it is possible to test this #[pallet::storage] macro :( unfortunately have not found any related examples

tests are in frame/support/test/tests/pallet.rs mostly

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3094970

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3094971

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3094969

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3096258

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3098444

@aaronbassett not sure, because I have found only one usage of these methods for the CountedMap https://github.com/Mr-Leshiy/substrate/blob/687ddfadfe61985ab94c852ac99c0b4794ca8391/frame/bags-list/remote-tests/src/snapshot.rs#L25 .
But maybe someone else can elaborate on this one

Only one usage of these methods within the Substrate codebase. It's impossible to know if developers are using them in other projects. To ensure the best possible developer experience, we should strive to provide a consistent interface with as few surprises as possible.
But, if we believe those methods on CountedMap are not used and provide no benefit, they should be deprecated and removed. However, if that is the case, then they should either be implemented in CountedNMap and then deprecated alongside the CountedMap methods, or this PR should not be merged until after the deprecation process has finished and the methods have been removed from CountedMap.
I don't know of any plans to deprecate the methods on CountedMap, and I think CountedNMap could be a really useful addition, so I would much rather it was the former.

@aaronbassett not sure, because I have found only one usage of these methods for the CountedMap https://github.com/Mr-Leshiy/substrate/blob/687ddfadfe61985ab94c852ac99c0b4794ca8391/frame/bags-list/remote-tests/src/snapshot.rs#L25 .
But maybe someone else can elaborate on this one

@Mr-Leshiy only one usage of these methods within the Substrate codebase. It's impossible to know if developers are using them in other projects. To ensure the best possible developer experience, we should strive to provide a consistent interface with as few surprises as possible.
But, if we believe those methods on CountedMap are not used and provide no benefit, they should be deprecated and removed. However, if that is the case, then they should either be implemented in CountedNMap and then deprecated alongside the CountedMap methods, or this PR should not be merged until after the deprecation process has finished and the methods have been removed from CountedMap.
I don't know of any plans to deprecate the methods on CountedMap, and I think CountedNMap could be a really useful addition, so I would much rather it was the former.

This is useful. However, I don't know if using counters is an anti-pattern for blockchain.
In my use case, say I have a structured Collection and Items.
I want to store how many items are in the collection,
I need to maintain a counter, which I may use to limit the max items per collection. so please don't deprecate CountedStorageMap
And I need to build indexes, for example index users' items in a collection.
I will do like this
#[pallet::storage]
pub type Account<T: Config<I>, I: 'static = ()> = StorageNMap<
	_,
	(
		NMapKey<Blake2_128Concat, T::AccountId>, // owner
		NMapKey<Blake2_128Concat, T::CollectionId>,
		NMapKey<Blake2_128Concat, T::ItemId>,
	),
	(),
	OptionQuery,
>;

(This piece of code is from https://github.com/paritytech/substrate/blob/master/frame/nfts/src/lib.rs#L219-L228 )
Further, I want to limit the max items of a user in a collection, so if I have CountedNMap, that's helpful.
Manual maintenance counter fields need very careful or easy to make bugs like the counter overflow, and the code will become complex and hard to read.
I'm very +1 for the feature!

only one usage of these methods within the Substrate codebase.

And there're 10 usages of CountedStorageMap in Substrate codebase, not only one

@jasl nobody is talking about deprecating CountedStorageMap, only either adding the counter_storage_final_key and map_storage_final_prefix methods to CountedNMap or removing those methods from CountedStorageMap so that there is consistency.

@jasl nobody is talking about deprecating CountedStorageMap, only either adding the counter_storage_final_key and map_storage_final_prefix methods to CountedNMap or removing those methods from CountedStorageMap so that there is consistency.

Ok, sorry for my misunderstanding

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3151119

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3152547

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3152630

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3153158

@thiolliere done, I have added and updated tests

bot fmt

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-benches
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3195167

kindly ping

bot fmt

@juangirini
Sorry for bothering you, but is this mergeable now?

@sam0x17 @KiChjang There were some changes since your las reviews, would you give it another look before merging?

Could it be merged? this is really an awesome feature

bot merge

bot rebase

bot merge

/tip medium

@Mr-Leshiy interested in a tip?

@kianenigma would be great :)
16GBceb9xiWB1R5YQGRfEYDZbWcatmLz8MPd6KZYpy4qfV7B

/tip medium


dealing with new derived input files

Right now, looper is still checking for input file existing based on the (soon to be outdated) "data_source" and its derived "data_path" attributes. Now with derived columns, any column could be the input source.
Instead, I propose we switch the input file checks to check for all (populated) derived columns for files to exist.
Thoughts?

See also my comment in 1e08758760b8a12e4240ac736df5132a8871d122 regarding bsf-specific checks.

Could you please explain what are derived columns?  I tried to explain it earlier in the release note e-mail.
but it's simple: just variable columns set in the project config that act like the original "data_source" column. now you can have as many as you want.
it's very useful for, for example, samples with read1/read2 split fastq inputs, and in the future, for multi-sample comparison tables. it makes looper extremely flexible, you can run it on any entities now, not just samples.

Okay. One or two examples of practical usage would be very useful though.

I made a major update to try to explain this. What do you think?
http://looper.readthedocs.io/en/latest/advanced.html#pointing-to-flexible-data-with-derived-columns

It's great!
Regarding your initial question (should we check the existence of files in all columns generated by derived columns?), I think this actually can be used to produce variables that aren't necessarily files right?
For example, I could have the root directory of a sample, or some file with stats specified in this way. Unlikely to be used, but a possibility.

Yes, you're right. I've only used it for files so far. "some file with stats" is still a file, though, and so is "root directory for a sample" (if a directory qualifies as a file). Still files, if not input files. Can you think of anything that would not exist?
And the more I use looper, the more I realize it is more flexible than we originally realized, so I think we should keep it flexible if we can.
But this means you can't really systematically check for input files existing, right?

The examples I gave are path to files, yes - but they might not yet exist at the time the sample is instantiated (i.e. produced during the pipeline run or in a later analysis), so checking them might not make much sense at that point.

But this means you can't really systematically check for input files existing, right?

Yes, I guess this is the compromise. We probably need to make the user aware that only data_source is interpreted as the path to the input file and that is checked.

But if these files are to be created later, should they really be in the annotation sheet list in the first place?
2nd idea: what about just eliminating the file checks altogether?
I would vote for either: all derived columns are checked; or, there are no checks. I see no reason to treat "data_source" differently.

Well, they really aren't in the annotation sheet, they are built from variables that are there - for someone that could be advantageous.
Anyway if it is between those two choices you might as well check all files then.

Yes, you're right.
What do you suggest? what about having a parallel section alongside derived_columns called built_files or something that aren't checked?
I guess the basic advantage of file checks is that jobs without all input files won't be submitted.
So there needs to be a way in the project sheet to specify which files are inputs and which are not

Which of the variables holds a value for the input files is an independent issue from checking all derived columns for the existence of a file.
I propose we make this configurable (through the project config) but with a default data_sources, similar to the results_subdir and submission_subdir. This would assure both backward compatibility and improved configurability.
We can then check either only that for the existence of input files, or all - no big preference.

Ok good. then I propose a new config attribute called required_inputs.
by default this can have data_source as a required input (although I question if this should be a default), and the user can put any other columns in there.
this is a nice solution because it also doesn't require you to use derived columns at all, if you want to hard code paths for example.
next question then is what is the default for required_inputs ? null or data_source? I vote for null so any required inputs must be made explicit.

Are there any other comments or suggestions relating to this proposal? ideas from @fhalbritter @jklughammer @cdietzgit ? Is the proposed solution acceptable?

If we make the required_inputs an empty list by default (which I guess is what you propose with null), then we have to always specify it, which goes against our current "minimal sensible defaults" phylosopy to have a minimal config.
data_source though is a default like any other, but is already in use and I think we should stick to that.

Also, I think right now, there is some confusion in Sample.set_file_paths() regarding this.
The first section calling Sample.locate_data_source() uses data_source as the keyword to expand paths as before, then the same for each derived column (which apparently the user would specify in the prj config). Then the section handling required_inputs only gathers the attributes of the sample which are in required_inputs, but this is not used anywhere else in the code. Very confusing.
How should we sort this out? I think we need to separate expanding paths from derived columns from which of these holds a path to the input files(s) somehow.

No... if there is no required_inputs, then looper will not bail on anything... it won't have to be specified. Actually, this is better because it means you don't require any particular columns, making minimal requirements extended to the sample annotation.
You only have to specify required inputs if you want it to fail if something doesn't exist. Actually now that I think about it, this should be specified by the pipeline, not by the project, right?
The code is confusing because I only ever partially implemented required_inputs, sorry. I was waiting for feedback above), which as you can see, never came.
So, here's a new concept: required_inputs is not in project config at all, but in pipeline_interface, specified by the pipeline author. Actually, this makes way more sense, because different data types may require different inputs, and it's the pipe that will determine; so projects with different data types will not be amenable to the direction we were going of a required_inputs attribute in project_config.

So, pipeline_interface would have something like:
rrbs.py:
  name: RRBS
  looper_args: True
  arguments:
    "--sample-name": sample_name
    "--genome": genome
    "--input": data_path
    "--single-or-paired": read_type
  required_inputs: [data_path]

required_inputs would be optional. Maybe it's better called required_files.
for pipelines that can take either 1 or 2 inputs (like rnaBitSeq), there's no good answer, because it depends on the same itself, whether it should require 1 or 2 inputs. I think there's no solution to that.
The automatic read-length checks would just go against anything in requried_inputs that is named *.bam or *.fastq or *.fastq.gz
But keep in mind, inputs could be anything... it might be a .hic file, for instance, for a secondary pipeline.

See also: https://github.com/epigen/pipelines/issues/34

@afrendeiro I think this is the last remaining pending thing that I think we should just solve before we merge... any thoughts?

I have a working implementation of this now. check it out.

Okay let's see if i understood:
Different pipelines might need different inputs (e.g. 1 file, 2+ files). In your implementation the pipelines state this and require the user to have to have (or build with derived columns) attribute(s) that are required by the pipelines and will be used as input file by them. Am I correct?
This is somewhat complicated. A few questions:

How will the user know what attribute(s) each pipeline needs to specify them in the annotation sheet? <- this seems critical to maintain simplicity, or else building a sample sheet will be a nightmare
Your implementation focuses on the PipelineInterface object to handle this. How will this interface with the sample objects? sample.data_path seems obsolete from now on:

what will happen to sample.locate_data_source? (and other sample functions that rely on its input files)
can we assign sample.data_source to a list of required_inputs (that would be specific for that sample type since these come from the pipelines) so that we can always get the input files of that sample without knowing specifically what files are required by the pipeline of that sample? (might be useful also for things like get input file sizes, etc...)

what happens if a pipeline wants to accept either 1 or more files for example?

Am I correct?
You are correct. Input files (can) vary by pipeline, so the pipeline must state this.

It feels complicated because we made all our pipelines standardized so they require exactly 1 input, and all exactly the same input. But most people won't do this (it restricts what the pipelines can do).

How will the user know what attribute(s) each pipeline needs to specify

When a user wants to use a pipeline, the user must know what inputs that pipeline takes. There is no way around this... short of specifying "all pipelines must take exactly 1 input, in a column called data_source", there is no way around this complexity. But that solution is severely limiting for pipeline authors. My pipelines no longer do this; hence the need for this new thing. Essentially, each pipeline must say "You must define these columns". This doesn't seem to difficult... For our suite of pipelines, we can make it simple: "you must define a data_source column" if that's what we want. But now, others can say their pipelines require different inputs. What about a pipeline that compares HiC data to H3K27ac data? It needs 2 inputs with very different characteristics. This is not possible with the old fixed system.

Your implementation focuses on the PipelineInterface object...sample.data_path seems obsolete from now on:

PipelineInterface reads the pipeline_interface.yaml -- then all it needs is a sample object and it can determine if the sample has everything needed for any given pipeline. This is how I implemented it, it seems to make sense to me...Indeed -- sample.data_path is now obsolete (and I suggest removing it in favor of the more universal and generic derived columns) -- but sample.data_source could remain a default derived column to the safe effect from the user point of view. So this stuff is all still in there; (locate_data_source or something equivalent is still used, now more generically, for all columns listed in derived_columns, which defaults to data_source).

can we assign sample.data_source to a list of required_inputs

This is just conceptually flawed because required_inputs is not a sample attribute, but a combined attribute relative to both a sample and a pipeline. In other words, we can't set required inputs for a sample unless we know what pipeline is being run. 1 sample can run 2 pipelines, and the required inputs differ; therefore, we shouldn't set that as a sample attribute. it's the wrong way to think about it. nevertheless, we can temporarily set it for convenience when working with a particular pipeline/sample pair... but we have to realize this is an instantiation of a sample with a particular pipeline run and not a generic sample.

what happens if a pipeline wants to accept either 1 or more files for example?

for pipelines accepting 1 or more files; the 1 file is listed as required, and this is all that is checked for pipeline submission. The other files are listed in the interface under all_input_files so these can be tallied for total input size. This highlights another issue solved by this method: the input size calculation can now be tailored to any inputs, as specified by the pipeline author (defaults to anything listed as a required input).
Another thing this solves is thus: inputs are relative to a pipeline run, not to a sample. 1 sample may have 2 input files (like, a microarray experiment and a sequencing experiment). Different pipelines may require different of those inputs. You don't want the Project object to check these at instantiation; you only want it to check when it's about to submit a pipeline. In short, Projects should be decoupled from input files, to make them totally generic; samples can be anything; maybe don't even have inputs. Inputs only become required when we want to do some action (a pipeline) on an input, at which point, the inputs are evaluated because they vary based on the action.
I could explain this more simply in person, but in the end, I think this does not add any new complexity for the user, while greatly increasing flexibility for pipeline authors. It should seem straightforward that to run a given pipeline on a given sample, that sample must define all the inputs required for that pipeline, and that this can vary by pipeline. In addition, at submission, the pipeline can give an easy-to-follow reminder ("you submitted the xyz pipeline, which requires a column named blahblah, but this did not exist in your input file, so I am not submitting this pipeline") -- very easy for the user to figure out what to include for samples that need that pipeline.
also enables secondary pipelines that run on outputs from earlier pipelines more easily.

(The reason why I prefer this type of media is that these discussions will be available to consult later and are searchable. Also at least in my case it also forces me to be much more concise and structured in the discourse.)
Okay I understand.
However, I'd like to comment on the development process of this - please take it as constructive criticism as that's my intention: When modifying/implementing a feature we should be aware of all its implications. I think it's good practice to avoid loose ends of code (pieces of code/functions no longer used after) and redundancy (by modifying code) instead of writing a parallel implementation.
This is obviously difficult but let's make an effort to keep the project tidy. Maybe something like this could be helpful: http://pycallgraph.slowchop.com/en/master/

Yes, this is starting to get a bit out of hand here. I have started a new branch and it is taking me deeper than I anticipated -- but I did put it in a new branch, so it causes no problems to anyone as I work.
Also some of the loose ends are because it's in process... the concepts are still being worked out and that creates confusion. When things resolve, the code can become much cleaner if we take the time to re-structure based on the final state. I'm working on that now, but it can always be improved. I'm reluctant to take stuff out immediately in case it ends up useful shortly, but it should be removed eventually. but anyway, it's always in the git history.
But what specifically are you referring to, as far as parallel implementation? Specific suggestions welcome of course.

If you could give this a try and see if you can get it to work and make sense, that would be helpful. Then I can go through and clean up the preliminary stuff that is no longer necessary. Here are some notes:
The idea is that a sample is first created with all of its universal attributes. Later, additional attributes that are relative to a particular pipeline will be added. This enables different pipelines to specify different requirements for the same sample. Previously this was not possible. It wasn't a problem, because all of our pipelines use the exact same input type: a single bam file. But for flexibility, we have to realize that other pipelines could conceivably use different types of input, including multiple different input types in a single pipeline. There is no reason looper can't also handle these types pipelines easily, and including the simple case.
So, I have now created three new configuration options that can be added to the pipeline interface config file. These are optional, and the system well try to default to something sensible if they are not provided. They are:
required_input_files: this is a list of file attributes that the pipeline requires. In the past, this was always just data path. Now, the pipeline author can define which attributes are required. If a sample does not have one of the attributes included in this list, the pipeline will not be submitted. This defaults to empty, meaning there is no required input file. This does not add any difficulty for a pipeline user, and just requires a single new definition for a pipeline author. Each pipeline specifies what it requires in the interface.  Our pipelines will add required_input_files: [data_path] to the interface and then work as before.
all_input_files: in the event that a pipeline has input files that are not required, yet should still be counted for the file size calculation that determines resource use, the pipeline author may add an optional pipeline interface configuration attribute to specify all input files. Looper will use these attributes to sum up the total input size for resource allocation. By default, this will simply be exactly the same as required_input_files. If neither is specified, it will be impossible for looper to know what attributes are input files, and therefore will not be able to request appropriate resources; so it's important for a pipeline author to define at least 1 of these two.
ngs_inputs: since some pipelines may require sequencing data, like ours, but other pipelines may require different types of input, we cannot assume that we can run read type or read length calculations on inputs generically. Instead the pipeline author must specify which attributes contain NGS files. Looper will use these files to assess read length and read type automatically. If no NGS input attributes are specified in the pipeline interface, looper will simply not automatically populate these attributes.
These are three fairly simple additions to the pipeline interface which are entirely optional, but provide excellent customizability for a pipeline author.
In practice what happens, is that looper will assess these interface-specific sample questions as it submits each pipeline, allowing this to be independent from the initial sample creation, such that each pipeline submission has its own evaluation of these attributes. This way, a sample that satisfies one pipeline but not another will still be able to submit the pipelines that it satisfies, and the others will simply not be submitted. This means that at project creation, it doesn't make sense to check sample for sample input, so this has been removed.
Finally, it had not previously been implemented that derived columns could populate columns in a merge table. This now works.

Okay sounds very good. I've had a look at the code and also ran some microtest samples and it all went okay.
Just a question: if a pipeline does not specify required_input_files in the interface, the sample will not obviously be checked for those attributes, but it will still be submitted to that pipeline anyway, right?
What I meant before with parallel implementations was more "duplicate" implementations such as the new in the inputs branch and the previous Sample.check_valid, because I though both were still being used just in different parts of the code. I guess Sample.check_valid is obsolete now and locate_data_source should probably be renamed to something like expand_derived_attributes/columns since it is generic and not only for generating the path to the input data.

Yes, there were some duplicates in there in my intermediate commits. I think I've cleaned all that up now.

I guess Sample.check_valid is obsolete now
check_valid is different, though... but it is a bit of relic, since we used to check more stuff. If you can simplify it away, great. right now it's still being used to check for sample_name -- different from file checks.

Just a question: if a pipeline does not specify required_input_files in the interface, the sample will not obviously be checked for those attributes, but it will still be submitted to that pipeline anyway, right?

The sample will not be checked for any input files and will therefore be submitted. It will have to have all attributes used  in the "arguments" section -- that's independent of input file checks.

locate_data_source should probably be renamed
agreed.

Renaming

Go ahead and make any changes you want and when you're done, I'll merge everything back into master and tag as 0.4-rc1 on Monday -- we can give some time for people to test that release candidate before finalizing it to 0.4, if everything works as expected.

In version 0.4, required_inputs became relative to a pipeline submission; data_path became deprecated, and data_source because the default derived_column. This issue is solved.

I started working on the looper.Could you please share is there any video tutorials(Links) for Looper.

@vmsiva There are no video tutorials. Your best bet is the documentation: http://looper.readthedocs.io/
PS we're getting ready to release version 0.6 next week so that might be a good time to start


Stable audio

@lukiod why was this closed 🤔?

Want to open in lukiod main to merge latest changes it was opened on livepeer main


Minify dependencies and remove junk files

The shipped dependencies, available under macOS at /Applications/Visual Studio Code.app/Contents/Resources/app/node_modules.asar, are unminified and all sorts of junk files are in there (readmes, licenses...).
Minifying everything and avoiding shipping unnecessary files seems like a fairly decent optimization opportunity.

I think there is history here but I don't know who has context, @jrieken?

I think @alexdima did the asar work

The history is that the junk used to be shipped as individual files. Now the junk is shipped in an .asar file, which speeds things up w.r.t. to install times and start-up time...
But the junk could be webpacked.


Detector bug from REST generator

I received the following error at the end of a run of probe Glitch100 using the REST generator:
File "/Users/erichacker/Library/Python/3.9/lib/python/site-packages/garak/detectors/base.py", line 159, in detect
   if trigger.lower() in output.lower():
AttributeError: 'list' object has no attribute 'lower'

I suspect that output was empty somehow. My python skills are weak.
Logs:
2023-11-13 15:17:01,872  INFO  invoked with arguments Namespace(model_type='rest', model_name=None, seed=None, generations=10, probes='glitch.Glitch100', detectors='', buff='', eval_threshold=0.5, deprefix=True, plugin_info=None, list_probes=False, list_detectors=False, list_generators=False, list_buffs=False, version=False, verbose=0, generator_option_file='rest_chatiq_dev.json', generator_options=None, probe_option_file=None, probe_options=None, report_prefix='', narrow_output=False, report=None, extended_detectors=False)
2023-11-13 15:17:01,888  INFO  started at 2023-11-13T15:17:01.852388
2023-11-13 15:17:01,888  INFO  reporting to garak.39477038-875f-46c0-8b07-14e46be375a1.report.jsonl
2023-11-13 15:17:02,009  INFO  generator init: <garak.generators.rest.RestGenerator object at 0x10688d9d0>
2023-11-13 15:17:02,011  DEBUG  harness run: <garak.harnesses.probewise.ProbewiseHarness object at 0x106b8f2e0>
2023-11-13 15:17:02,011  INFO  probe queue: probes.glitch.Glitch100
2023-11-13 15:17:02,012  INFO  probe init: <garak.probes.glitch.Glitch100 object at 0x106c554f0>
2023-11-13 15:17:02,014  INFO  detector init: <garak.detectors.goodside.Glitch object at 0x106c56100>
2023-11-13 15:17:02,014  DEBUG  harness run: <garak.harnesses.base.Harness object at 0x1075e1c70>
2023-11-13 15:17:02,014  INFO  generating...
2023-11-13 15:17:02,014  DEBUG  probe execute: <garak.probes.glitch.Glitch100 object at 0x106c554f0>

And then 2000 lines of Connection/Responses, all 1000 tests ran. The report file looks complete.
Let me know if there is anything else you need to troubleshoot.

Thanks for reporting. Can you send a longer exception trace?

Hey Eric,
Would love to fix this - a longer exception, or a command line that reproduces the error, is needed to help us get further


Add license-check and vulnerability-check inputs

Closes #308
This add support for and document two new inputs:

license-check: Can be used to disable all license-related checks performed by this action.
vulnerability-check: Can be used to disable all vulnerability-related checks performed by this action.

A check is included that prevents both checks to be disabled since this would render this action useless.

@ericcornelissen This looks great, thanks a ton! The changes will go into the next release of the Action (sometime this week).


Launching a Dialog from a Dialog causes errors in v2_develop

Describe the bug
First encountered this bug in https://github.com/gui-cs/Terminal.Gui/pull/2259#issuecomment-1475190039
Here is a minimum repo:
Application.Init ();
var btn1 = new Button ("press me 1");
var btn2 = new Button ("Show Sub");
var btn3 = new Button ("Close");
btn3.Clicked += () => Application.RequestStop ();
btn2.Clicked += () => { MessageBox.Query ("hey", "ya", "ok"); };
var dlg = new Dialog ("Hey", btn2,btn3);
btn1.Clicked += ()=>Application.Run (dlg);
var w = new Window ();
w.Add (btn1);
Application.Run (w);

Application.Shutdown ();

To Reproduce
Steps to reproduce the behavior:

Click button
Click 'show sub'
Close sub

Expected behavior
Sub should close leaving the Dialog still showing but instead the sub border still shows and system becomes unstable.
Screenshots

I already reverted all my PR but the still exist. I'll investigate it.

@tznind this should be like this because the dialog is destroyed when exit and if you want to run again you must recreate it.
	Application.Init ();
	var btn1 = new Button ("press me 1");
	btn1.Clicked += () => {
		var btn2 = new Button ("Show Sub");
		var btn3 = new Button ("Close");
		btn3.Clicked += () => Application.RequestStop ();
		btn2.Clicked += () => { MessageBox.Query ("hey", "ya", "ok"); };
		var dlg = new Dialog ("Hey", btn2, btn3);

		Application.Run (dlg);
	};
	var w = new Window ();
	w.Add (btn1);
	Application.Run (w);

	Application.Shutdown ();

But the issue remain, I think that is because the Border is now like a static property due the configuration and the previous dialog has the property of the MessageBox border property after closed. I already realized that something is wrong because of the static configurations which I would like @tig can say something about.

What is going on with this issue is the fact that the sub-dialog is overwriting the first Dialog.


Validate data size for proposals only

There are no watchdogs and it makes no sense to limit triggers by size (invalid triggers are going to be voted down anyway).
512 bytes should be more than enough for any proposal - the only value we do not check size of is URL and with 512 bytes total limit URL has space for almost 300 bytes.

Do we need to add a cap to the proposal size in Sentinel also (and replace the MAX_GOVERNANCE_OBJECT_DATA_SIZE check)?
I think we should have consistent validation rules in both Sentinel and Core.

Yes, I guess we do.

I have a couple more questions on this:

Since this is the only place in the code actually using this value, can we remove MAX_GOVERNANCE_OBJECT_DATA_SIZE entirely?

Since it's not being used after this PR anyway, do we want to update the value instead of replace as I suggested ^ and allow Sentinel to poll for this proposal max size, or should we just remove the extra JSONRPC code and keep Sentinel in-sync manually? (Normally I would not suggest this, but pro's for this approach include removing extra cruft and the fact that Sentinel is largely in maintenance mode and will be removed someday.)

I guess not yet, we still use it when we communicate with old nodes https://github.com/dashpay/dash/blob/develop/src/governance-object.h#L313-L322
See (1) for not used part. We can expose proposal max size in the same RPC  but having it hardcoded won't hurt too I guess - I doubt we are going to change this value anytime soon and we won't be doing it very often even if we had to tweak it for some reason.


Adding strong typing that passes flow type checks in one file and a p…

…rettify update to another file.

Summary

Adding strong typing that passes flow type checks in one file and a prettify update to another file.
Test plan

Enable the "no-weak-types" rule in .eslintrc and run:  yarn eslint {path_to_project}/packages/jest-phabricator/src/index.js.  Disable the "no-weak-types" rule in .eslintrc and run: yarn test.

Thank you! I don't think that's the correct prettier update. Do you mind running yarn and reverting the change to babel-jest?

Change to babel-jest had been reverted.
On Thu, Apr 27, 2017, 4:30 PM Christoph Pojer<EMAIL_ADDRESS>wrote:

Thank you! I don't think that's the correct prettier update. Do you mind
running yarn and reverting the change to babel-jest?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/facebook/jest/pull/3393#issuecomment-297866952, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADQUB8hKbG4lFJG0320PRw658anQf8KWks5r0SUSgaJpZM4NK4p7
.

Thanks, happy to be here :)  Not sure where the last two comments on the pull request went, but I made the suggested update to the function return.  yarn test runs.

Thank you!


source/core/screen.c:140:25: error: too few arguments to function 'fontGetGlyphInfo'

When I attempt to compile FBI v2.6.0 I get this error:
source/core/screen.c: In function 'screen_init':
source/core/screen.c:140:25: error: too few arguments to function 'fontGetGlyphInfo'
140 |     TGLP_s* glyphInfo = fontGetGlyphInfo();
|                         ^~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:191:23: note: declared here
191 | static inline TGLP_s* fontGetGlyphInfo(CFNT_s* font)
|                       ^~~~~~~~~~~~~~~~
source/core/screen.c:151:42: warning: passing argument 1 of 'fontGetGlyphSheetTex' makes pointer from integer without a cast [-Wint-conversion]
151 |         tex->data = fontGetGlyphSheetTex(i);
|                                          ^
|                                          |
|                                          int
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:203:50: note: expected 'CFNT_s ' {aka 'struct  '} but argument is of type 'int'
203 | static inline void fontGetGlyphSheetTex(CFNT_s font, int sheetIndex)
|                                          ^~
source/core/screen.c:151:21: error: too few arguments to function 'fontGetGlyphSheetTex'
151 |         tex->data = fontGetGlyphSheetTex(i);
|                     ^~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:203:21: note: declared here
203 | static inline void* fontGetGlyphSheetTex(CFNT_s* font, int sheetIndex)
|                     ^~~~~~~~~~~~~~~~~~~~
source/core/screen.c: In function 'screen_get_font_height':
source/core/screen.c:494:21: error: too few arguments to function 'fontGetInfo'
494 |     return scaleY * fontGetInfo()->lineFeed;
|                     ^~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:180:23: note: declared here
180 | static inline FINF_s* fontGetInfo(CFNT_s* font)
|                       ^~~~~~~~~~~
source/core/screen.c: In function 'screen_wrap_string':
source/core/screen.c:561:80: warning: passing argument 1 of 'fontGlyphIndexFromCodePoint' makes pointer from integer without a cast [-Wint-conversion]
561 |         charWidth = scaleX * fontGetCharWidthInfo(fontGlyphIndexFromCodePoint(code))->charWidth;
|                                                                                ^~~~
|                                                                                |
|                                                                                u32 {aka long unsigned int}
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:41: note: expected 'CFNT_s ' {aka 'struct  '} but argument is of type 'u32' {aka 'long unsigned int'}
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|                                 ^~
source/core/screen.c:561:52: error: too few arguments to function 'fontGlyphIndexFromCodePoint'
561 |         charWidth *= scaleX * fontGetCharWidthInfo(fontGlyphIndexFromCodePoint(code))->charWidth;
|                                                    ^~~~~~~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:5: note: declared here
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
source/core/screen.c:561:31: error: too few arguments to function 'fontGetCharWidthInfo'
561 |         charWidth = scaleX * fontGetCharWidthInfo(fontGlyphIndexFromCodePoint(code))->charWidth;
|                               ^~~~~~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:223:18: note: declared here
223 | charWidthInfo_s fontGetCharWidthInfo(CFNT_s font, int glyphIndex);
|                  ^~~~~~~~~~~~~~~~~~~~
source/core/screen.c:566:31: error: too few arguments to function 'fontGetInfo'
566 |                 lh = scaleY * fontGetInfo()->lineFeed;
|                               ^~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:180:23: note: declared here
180 | static inline FINF_s* fontGetInfo(CFNT_s* font)
|                       ^~~~~~~~~~~
source/core/screen.c:603:27: error: too few arguments to function 'fontGetInfo'
603 |             lh = scaleY * fontGetInfo()->lineFeed;
|                           ^~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:180:23: note: declared here
180 | static inline FINF_s* fontGetInfo(CFNT_s* font)
|                       ^~~~~~~~~~~
source/core/screen.c: In function 'screen_draw_string_internal':
source/core/screen.c:695:69: warning: passing argument 1 of 'fontGlyphIndexFromCodePoint' makes pointer from integer without a cast [-Wint-conversion]
695 |                 fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(code), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                                                                     ^~~~
|                                                                     |
|                                                                     u32 {aka long unsigned int}
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:41: note: expected 'CFNT_s ' {aka 'struct  '} but argument is of type 'u32' {aka 'long unsigned int'}
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|                                 ^~
source/core/screen.c:695:41: error: too few arguments to function 'fontGlyphIndexFromCodePoint'
695 |                 fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(code), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                                         ^~~~~~~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:5: note: declared here
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
source/core/screen.c:695:17: error: too few arguments to function 'fontCalcGlyphPos'
695 |                 fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(code), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                 ^~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:234:6: note: declared here
234 | void fontCalcGlyphPos(fontGlyphPos_s* out, CFNT_s* font, int glyphIndex, u32 flags, float scaleX, float scaleY);
|      ^~~~~~~~~~~~~~~~
source/core/screen.c:698:73: warning: passing argument 1 of 'fontGlyphIndexFromCodePoint' makes pointer from integer without a cast [-Wint-conversion]
698 |                     fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(0xFFFD), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                                                                         ^~~~~~
|                                                                         |
|                                                                         int
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:41: note: expected 'CFNT_s ' {aka 'struct  '} but argument is of type 'int'
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|                                 ^~
source/core/screen.c:698:45: error: too few arguments to function 'fontGlyphIndexFromCodePoint'
698 |                     fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(0xFFFD), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                                             ^~~~~~~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:216:5: note: declared here
216 | int fontGlyphIndexFromCodePoint(CFNT_s font, u32 codePoint);
|     ^~~~~~~~~~~~~~~~~~~~~~~~~~~
source/core/screen.c:698:21: error: too few arguments to function 'fontCalcGlyphPos'
698 |                     fontCalcGlyphPos(&data, fontGlyphIndexFromCodePoint(0xFFFD), GLYPH_POS_CALC_VTXCOORD, scaleX * font_scale, scaleY * font_scale);
|                     ^~~~~~~~~~~~~~~~
In file included from /opt/devkitpro/libctru/include/3ds.h:91,
from source/core/screen.c:6:
/opt/devkitpro/libctru/include/3ds/font.h:234:6: note: declared here
234 | void fontCalcGlyphPos(fontGlyphPos_s* out, CFNT_s* font, int glyphIndex, u32 flags, float scaleX, float scaleY);
|      ^~~~~~~~~~~~~~~~
source/core/screen.c: In function 'screen_get_font_height':
source/core/screen.c:495:1: warning: control reaches end of non-void function [-Wreturn-type]
495 | }
| ^
make: *** [buildtools/make_base:722: build/3ds-arm/source/core/screen.o] Error 1
rm build/3ds-arm/source/core/default.shbin.c

This appears to be fixed in the github repo but not the v2.6.0 release of the source. Marking as closed.


Straightforward migration on Swift 4

Codecov Report

Merging #5 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
+ Coverage   99.27%   99.27%   +<.01%
==========================================
  Files           6        6
  Lines         274      276       +2
==========================================
+ Hits          272      274       +2
  Misses          2        2

Impacted Files
Coverage Δ

Sources/Progress.swift
100% <100%> (ø)
:arrow_up:

Sources/Utilities.swift
94.11% <100%> (ø)
:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 694fe3a...3cf621c. Read the comment docs.

awesome. thank you!


added calendar-button-icon-content

forgot to update the README.md. sorry. Here is the calendar-button-icon-content with a example. "calendar-button-icon" has to be set to "material-icons" as well in order to use the material icon to work.

Coverage remained the same at 98.951% when pulling 7f2f08148949fea5d691dbdae233bd5e288ef636 on tobischulz:patch-2 into 18dcd7ad28663b3289cbe79a16543a7cb6f8a2f1 on charliekassel:master.

Hi @tobischulz - can you add this commit to your other PR please

done


Capstone Project

Digital World

I personalize the content of my page. Choose a topic "Digital world" .
I have build 2 pages: home page and  about page.
The section "Featured speakers" I have created dynamically in JavaScript.
Video presentation link : https://www.loom.com/share/4a440ab37e49409086ea2caa8595ea73

Make changes as suggested.


Fix(sales): Fix observers for sales order and shipment

In both cases the increment_id was written to the queue, although the entity_id was needed for the supplied consumers.

Strange, I am pretty sure that I tested these events when I implemented them...

i had a quick look and i'm feeling that might be the case as both creditmemos and invoices use the increment id while the repositories expect the entity id?
if you'd like to update that that'd be great, thanks!


Jackson-databind bump to 2.12.6

Description
Bumping up version for Jackson libraries to 2.12.6
Issues Resolved
[List any issues this PR will resolve]
Check List

[ ] New functionality includes testing.

[ ] All tests pass, including unit test, integration test and doctest

[ ] New functionality has been documented.

[ ] New functionality has javadoc added
[ ] New functionality has user manual doc added

[x] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Codecov Report

Merging #410 (643ce25) into main (b0ef5e0) will decrease coverage by 33.59%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##               main     #410       +/-   ##
=============================================
- Coverage     96.51%   62.91%   -33.60%
=============================================
  Files           264       10      -254
  Lines          7172      658     -6514
  Branches        538      118      -420
=============================================
- Hits           6922      414     -6508
+ Misses          196      191        -5
+ Partials         54       53        -1

Flag
Coverage Δ

query-workbench
62.91% <ø> (ø)

sql-engine
?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

.../org/opensearch/sql/sql/parser/AstSortBuilder.java

...earch/sql/data/utils/ReverseExprValueOrdering.java

...search/response/agg/NoBucketAggregationParser.java

...search/sql/data/model/AbstractExprNumberValue.java

...sql/expression/datetime/DateTimeFormatterUtil.java

...ript/aggregation/dsl/BucketAggregationBuilder.java

.../planner/logical/rule/PushProjectAndIndexScan.java

...search/sql/expression/config/ExpressionConfig.java

...esponse/format/VisualizationResponseFormatter.java

...search/data/value/OpenSearchExprGeoPointValue.java

... and 244 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0ef5e0...643ce25. Read the comment docs.

@penghuo @joshuali925 It looks like SQL Build is failing due to conflicting junit Versions.
OpenSearch uses junit 4.13.2 and our jackson needs junit 4.13.1.
> Task :integ-test:dependencyInsight
Dependency resolution failed because of conflict(s) on the following module(s):
   - junit:junit between versions 4.13.2 and 4.13.1

junit:junit:4.13.2
   variant "compile" [
      org.gradle.status              = release (not requested)
      org.gradle.usage               = java-api
      org.gradle.libraryelements     = jar (compatible with: classes)
      org.gradle.category            = library

      Requested attributes not found in the selected variant:
         org.gradle.dependency.bundling = external
         org.gradle.jvm.version         = 8
   ]
   Selection reasons:
      - By constraint
      - By conflict resolution : between versions 4.13.2 and 4.13.1

junit:junit:4.13.2
\--- org.opensearch.test:framework:1.3.0-SNAPSHOT:20220202.221337-280
     \--- testCompileClasspath

junit:junit:4.13.1 -> 4.13.2
\--- com.fasterxml.jackson:jackson-bom:2.12.6
     +--- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.12.6
     |    +--- project :protocol
     |    |    +--- project :ppl
     |    |    |    \--- project :plugin
     |    |    |         \--- testCompileClasspath
     |    |    \--- project :sql
     |    |         \--- project :legacy
     |    |              +--- testCompileClasspath
     |    |              \--- project :plugin (*)
     |    +--- project :opensearch
     |    |    +--- project :plugin (*)
     |    |    +--- project :legacy (*)
     |    |    \--- project :protocol (*)
     |    +--- org.opensearch:opensearch-x-content:1.3.0-SNAPSHOT:20220202.221206-280
     |    |    +--- project :ppl (*)
     |    |    \--- org.opensearch:opensearch:1.3.0-SNAPSHOT:20220202.221321-280
     |    |         +--- org.opensearch.test:framework:1.3.0-SNAPSHOT:20220202.221337-280
     |    |         |    \--- testCompileClasspath
     |    |         +--- org.opensearch.client:opensearch-rest-high-level-client:1.3.0-SNAPSHOT:20220202.221150-280
     |    |         |    \--- testCompileClasspath
     |    |         +--- project :legacy (*)
     |    |         \--- project :opensearch (*)
     |    \--- com.fasterxml.jackson:jackson-bom:2.12.6 (*)
     +--- com.fasterxml.jackson.core:jackson-databind:2.12.6
     |    +--- project :protocol (*)
     |    +--- project :opensearch (*)
     |    +--- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.12.6 (*)
     |    \--- com.fasterxml.jackson:jackson-bom:2.12.6 (*)
     +--- com.fasterxml.jackson.core:jackson-core:2.12.6
     |    +--- org.opensearch.client:opensearch-rest-client-sniffer:1.3.0-SNAPSHOT:20220202.221143-280
     |    |    \--- org.opensearch.test:framework:1.3.0-SNAPSHOT:20220202.221337-280 (*)
     |    +--- project :protocol (*)
     |    +--- project :opensearch (*)
     |    +--- org.opensearch:opensearch-x-content:1.3.0-SNAPSHOT:20220202.221206-280 (*)
     |    +--- com.fasterxml.jackson.dataformat:jackson-dataformat-cbor:2.12.6 (*)
     |    +--- com.fasterxml.jackson.core:jackson-databind:2.12.6 (*)
     |    \--- com.fasterxml.jackson:jackson-bom:2.12.6 (*)
     \--- com.fasterxml.jackson.core:jackson-annotations:2.12.6
          +--- com.fasterxml.jackson.core:jackson-databind:2.12.6 (*)
          \--- com.fasterxml.jackson:jackson-bom:2.12.6 (*)

Force to Junit 4.13.2?


MSS310H energy monitor

Hello,
the integration work flawless but the energy monitor in this device show Wh instead of kWh.
It's possible modified this value?

Hello @ster1um ,
I've decided in the beginning to report that in Wh since it was 'more' in range with my usual experience about the daily power consumption for the plug appliance. Nevertheless, I see it may be desirable sometimes to have the value in kWh.
I'll see if I can introduce a configuration flag for setting the energy unit to kWh instead of Wh

Hello @krahabb,
Thanks for quick response.
I know with daily use it doesn't not change anything.
If you in next release introduce this feature to switch from wh to kwh It would be greatly appreciated.

Hello @ster1um ,
I'm going to release an update in the coming days but I think the feature to choose the energy unit will be 'out of reach' not because it's too difficult to implement but for a 'phylosophic' reason:

Changing the entity 'unit' will modify the historic sensor data starting from when you apply the change so you'll be left with the older values (in Wh) and newer ones (in kWh) both of which will be interpreted from HA UI as the newer unit i.e. kWh. This way, the history panel would show a (big and erroneous) scale change in data. Also, the statistics, if enabled would be messed up since they're carried on the numeric values aswell and not on 'normalized' ones (statistics would then mix Wh and kWh without knowing there's a scale change)

I know this should turn out to be a user 'responsible' choice but I'm a bit scared of the issues that would come through anyway.
I think you could still setup a template sensor in order to scale the native meross_lan sensor values and accomplish this scale change 'cleanly' on a new entity
I'm nevertheless open to the discussion and I'm not closing this down with a 'will never do' statement ;)

I would like to put forward an alternative viewpoint.
I use MSS310 for monitoring and control of a number of households appliances; fridge, dryer, microwave and office using Meross_LAN, which works well. Daily device energy (Wh) are included in the HA energy dashboard (displayed as kWh) and I have a template to track the sum of instant power (W) of my household appliances. I find it very useful that the integration resets daily energy (Wh) consumption, but the energy dashboard then automatically calculates energy (kWh) over day, week, month, and yearly intervals, without the need for templates.
Thus going forward it is good if integration sensors reported in base units (in this case V, A, W & Wh) and the display layer (Lovelace or Dashboards) determine the relevant prefix k, M or whatever. Users can always do their own scaling though templates.