Access Controller

DRAFT:
@mbaker3 This is meant to be a more general case helper for managing write/read access to collections but I want to get your thoughts and feedback on it before making it a real PR because some of it feels a bit weird in terms of naming to me.
It's similar to the AbstractValueAccessController except that class assumes two things:

That the instance you are wrapping will always exist.

There are many times where I need to re-create or double buffer collections each frame and so I want my Access Control to be a bit more abstract and gating on the system level.

The Acquire/Release flow makes it so that you can't have multiple jobs scheduled to write or read all at the same time without managing some handles at the caller site yourself.

There are many times where I want to schedule jobs from different systems to all write at the same time. I need them all to kick off from one WriteHandle but I also need to track all the parallel writes so we know when it's safe to do a read.

So enter the AccessController.
The expectation is that you would GetOrCreate an instance at your system level. Or multiple systems that don't need to know about each other at all.
You can then schedule your jobs based on your needs.

Do you need to write?

If so, do you need your job to be exclusive in writing? Or can other jobs write at the same time?

Do you need to read?

If so, do you need your job to be exclusive in reading? Or can other jobs read at the same time?

Do you need to read/write?

Since the other systems dealing with the same collection are using the same instance of the AccessController, everyone gets scheduled properly without conflict while also getting out of the way as fast as possible at the system level.
What is the current behaviour?
This doesn't exist. See the manual managing on handles in TimerSubSystem on main here: main
What is the new behaviour?
This is now in use in two areas.

A DynamicBuffer test case. See WritePDynamicBufferSystemA, B, and C to see how three systems are all trying to write to the buffer at the same time. See AggregateDynamicBufferSystem for how it handles reading to aggregate once all the parallel writes are done.
Updated TimerSubSystem to use an AccessController instead of manually managing the handles.

examples
What issues does this resolve?

None

What PRs does this depend on?

Parallel Collection Util

Parallel Collection Util stuff is in this PR because the examples in 3fa472083fb47ea7558c0478e759c4e69077b435 rely on it.
Does this introduce a breaking change?

[ ] Yes
[x] No

Just had a thought. I should probably know this but is the built in dependency management done per-type or per chunk?
Like if one query is (in MyComponent, ref Translation, exclude YourComponent) and another is (in YourComponent, ref Translation, exclude MyComponent) can those execute at the same time?
If the dependencies do work per chunk then there may be unexpected downsides to using this controller with queries that only address some instance of a type.
If that's the case:

We might want a separate implementation for chunk based access control and instance based access control.
Maybe overly it's complex to support per chunk dependency management. Would almost certainly need to start handling queries.

@mbaker Converted this to a real PR now with all the updated changes for the new Acquire/Release flow.
The latest here: https://github.com/decline-cookies/twisted-dots/commit/18a438537b9a0afb1823f8d195cb2a9ac6497d0b
Shows real world examples in for the Health system and Timer system.

Oh I forgot to address your overall comments.
I thought about it and I don't believe we should have the CollectionAccessController responsible for managing dependencies with Queries or built in dependency management.
This gets used in two scenarios:

Native Collections that are meant for non-entity processing/data.
a. In this case (arguably the most common case for using this) there are no queries or built in dependency management possible anyway.
Wrapping a DynamicBuffer.
a. In this case, the only reason you would want to use the CollectionAccessController is because you are using a job with the NativeDisableContainerSafetyRestriction attribute on the DynamicBuffer which turns off the built in dependency management. For all other cases, the built in will take care of everything for you.
b. If you do use the NativeDisableContainerSafetyRestriction attribute, then you will control the systems that handle the Acquire/Release flow and you can combine the System Dependency values as needed to the handles so that the CollectionAccessController is part of the chain that the built in dependency management has built up.

To add the functionality to CollectionAccessController seems overkill and difficult. I would propose that if we find a third scenario, we could add it as a separate PR where's it's additional opt-in functionality.

Just had a thought. I should probably know this but is the built in dependency management done per-type or per chunk?
Like if one query is (in MyComponent, ref Translation, exclude YourComponent) and another is (in YourComponent, ref Translation, exclude MyComponent) can those execute at the same time?

I tested this.
Take the following setup:
if (i % 2 == 0)
            {
                Entity chunkTestA = entityManager.CreateEntity(ComponentType.ReadWrite<MyComponent>(),
                                                               ComponentType.ReadWrite<AlwaysComponent>());

                entityManager.SetComponentData(chunkTestA, new MyComponent()
                {
                    Value = 5.0f
                });
                entityManager.SetComponentData(chunkTestA, new AlwaysComponent()
                {
                    Value = 3.0f
                });
            }
            else
            {
                Entity chunkTestB = entityManager.CreateEntity(ComponentType.ReadWrite<YourComponent>(),
                                                               ComponentType.ReadWrite<AlwaysComponent>());

                entityManager.SetComponentData(chunkTestB, new YourComponent()
                {
                    Value = 1.0f
                });
                entityManager.SetComponentData(chunkTestB, new AlwaysComponent()
                {
                    Value = 3.0f
                });
            }

100,000 entities, 50,000 with MyComponent and AlwaysComponent and 50,000 with YourComponent and AlwaysComponent.
Queries like this:
            m_MyQuery = GetEntityQuery(ComponentType.ReadOnly<MyComponent>(),
                                       ComponentType.ReadWrite<AlwaysComponent>(),
                                       ComponentType.Exclude<YourComponent>());

            m_YourQuery = GetEntityQuery(ComponentType.ReadOnly<YourComponent>(),
                                       ComponentType.ReadWrite<AlwaysComponent>(),
                                       ComponentType.Exclude<MyComponent>());

Job scheduling like this for IJobEntityBatch:
            JobHandle myHandle = new MyJob(GetComponentTypeHandle<MyComponent>(true),
                                           GetComponentTypeHandle<AlwaysComponent>(false))
               .ScheduleParallel(m_MyQuery, 1, Dependency);

            JobHandle yourHandle = new YourJob(GetComponentTypeHandle<YourComponent>(true),
                                           GetComponentTypeHandle<AlwaysComponent>(false))
               .ScheduleParallel(m_YourQuery, 1, Dependency);

            Dependency = JobHandle.CombineDependencies(myHandle, yourHandle);

This cannot work because both jobs write to the AlwaysComponent. So it appears the built in dependency gate is happening on the Type, not the chunk.
If I flip it so that the AlwaysComponent is ReadOnly and the My and Your components are writeable, then both jobs can run at the same time because there is no conflict.
So in short, it's per-type not per-chunk

I don't think this is meant for me I'm not sure why I'm getting this
On Mon, May 2, 2022, 2:38 PM Jon Keon @.***> wrote:

Oh I forgot to address your overall comments.
I thought about it and I don't believe we should have the
CollectionAccessController responsible for managing dependencies with
Queries or built in dependency management.
This gets used in two scenarios:

Native Collections that are meant for non-entity processing/data.
a. In this case (arguably the most common case for using this) there
are no queries or built in dependency management possible anyway.
Wrapping a DynamicBuffer.
a. In this case, the only reason you would want to use the
CollectionAccessController is because you are using a job with the
NativeDisableContainerSafetyRestriction attribute on the DynamicBuffer
which turns off the built in dependency management. For all other cases,
the built in will take care of everything for you.
b. If you do use the NativeDisableContainerSafetyRestriction
attribute, then you will control the systems that handle the Acquire/
Release flow and you can combine the System Dependency values as
needed to the handles so that the CollectionAccessController is part
of the chain that the built in dependency management has built up.

To add the functionality to CollectionAccessController seems overkill and
difficult. I would propose that if we find a third scenario, we could add
it as a separate PR where's it's additional opt-in functionality.
Just had a thought. I should probably know this but is the built in
dependency management done per-type or per chunk?
Like if one query is (in MyComponent, ref Translation, exclude
YourComponent) and another is (in YourComponent, ref Translation, exclude
MyComponent) can those execute at the same time?
I tested this.
Take the following setup:
if (i % 2 == 0)
{
Entity chunkTestA = entityManager.CreateEntity(ComponentType.ReadWrite(),
ComponentType.ReadWrite());
            entityManager.SetComponentData(chunkTestA, new MyComponent()
            {
                Value = 5.0f
            });
            entityManager.SetComponentData(chunkTestA, new AlwaysComponent()
            {
                Value = 3.0f
            });
        }
        else
        {
            Entity chunkTestB = entityManager.CreateEntity(ComponentType.ReadWrite<YourComponent>(),
                                                           ComponentType.ReadWrite<AlwaysComponent>());

            entityManager.SetComponentData(chunkTestB, new YourComponent()
            {
                Value = 1.0f
            });
            entityManager.SetComponentData(chunkTestB, new AlwaysComponent()
            {
                Value = 3.0f
            });
        }

100,000 entities, 50,000 with MyComponent and AlwaysComponent and 50,000
with YourComponent and AlwaysComponent.
Queries like this:
        m_MyQuery = GetEntityQuery(ComponentType.ReadOnly<MyComponent>(),
                                   ComponentType.ReadWrite<AlwaysComponent>(),
                                   ComponentType.Exclude<YourComponent>());

        m_YourQuery = GetEntityQuery(ComponentType.ReadOnly<YourComponent>(),
                                   ComponentType.ReadWrite<AlwaysComponent>(),
                                   ComponentType.Exclude<MyComponent>());

Job scheduling like this for IJobEntityBatch:
        JobHandle myHandle = new MyJob(GetComponentTypeHandle<MyComponent>(true),
                                       GetComponentTypeHandle<AlwaysComponent>(false))
           .ScheduleParallel(m_MyQuery, 1, Dependency);

        JobHandle yourHandle = new YourJob(GetComponentTypeHandle<YourComponent>(true),
                                       GetComponentTypeHandle<AlwaysComponent>(false))
           .ScheduleParallel(m_YourQuery, 1, Dependency);

        Dependency = JobHandle.CombineDependencies(myHandle, yourHandle);

This cannot work because both jobs write to the AlwaysComponent. So it
appears the built in dependency gate is happening on the Type, not the
chunk.
If I flip it so that the AlwaysComponent is ReadOnly and the My and Your
components are writeable, then both jobs can run at the same time because
there is no conflict.
So in short, it's per-type not per-chunk
—
Reply to this email directly, view it on GitHub
https://github.com/decline-cookies/anvil-unity-dots/pull/20#issuecomment-1115232479,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ATEB5BX6NHZN4ALLMPWWO3DVIAOLNANCNFSM5UJOGF4Q
.
You are receiving this because you were mentioned.Message ID:
@.***>

@MBaker Bah, I'm sorry. I forgot this repo was public and auto-correct just gave your user instead of @mbaker3.
I'll be sure to remember in the future and I'll remove these comments referencing you. Sorry again!

Wrapping a DynamicBuffer.

Is there any benefit to using AccessController if you're not going to take advantage of the NativeDisableContainerSafetyRestriction? That's the core use case for this class unless I'm missing something.

No, I don't think so. The primary use for CollectionAccessController is for non-entity collections. The only entity collection is DynamicBuffer and this class helps you for the one case where you want to have parallel writes by different job types at the same time. That requires you to use NativeDisableContainerSafetyRestriction.

As a developer, I have to have a deep enough understanding of what's going on to know to only request read access on the built in system even though I'm going to write. If I ask for write access I negate the whole purpose of AccessController by blocking all other jobs from accessing until my work is complete.

I'd need to walk through an example with you but I don't think this is true. It doesn't matter what kind of access you request on the built in system if the job that uses it has the NativeDisableContainerSafetyRestriction.

By leaving it on the developer to combine the results into the built in dependency we're limiting the use of this parallel element writing to very tightly coordinated systems where the first system waits on the build in dependency and the last system writes the release handle into its dependency. Being careful that no system which uses the built in dependency system executes between that window.

Again I think we need to walk through an example, but I'm not seeing a developer do that in my mind? If for some reason you need to write to a specific DynamicBuffer in parallel, then you use the AccessController to get a shared write and off you go. If other systems existing or future come along with the same need, they do the same thing and it slots in nicely. No coupling. Each system can write their release handle into the system Dependency. It won't affect anything except making sure it's done before the next frame.
Whoever wants to read does the same thing and exclusive write too.
Other

Can we build an assert into base system(s) that prevents you from using the built in dependency management for a type that has an AccessController? (outlined in option 2 in previous comment)

Maybe? I'm not sure how we'd accomplish that and based on my other comments above, I don't think it's an issue. We need to come up with a test case that forces this issue to see if it actually is one.

@mbaker3
Ready for review again.

Generally remove the use of "we" and "our" in the docs

Fair, but if you search on it, it's throughout all of Anvil so I suggest we try to remember this going forward and then at some point do a pass through the library.

Makes sense. Looks like mostly your comments 😆

Should probably move all of the PlayerLoop/System cache into it's own directory and namespace. Maybe this is less of a cache and instead a SystemGraph?

Moved. In my mind Cache seems to make more sense since we're caching things that might change and we want to compare to what we had before. The SystemGraph seems more like a different way of transversal?

Sure, not updated graph is basically a cache. Snapshot would work too. The important part was getting it out into its own folder though so we're good.


Uncaught exception

Uncaught Exception:
TypeError: Cannot read property 'length' of null
at closeWindows (/Applications/stretchly.app/Contents/Resources/app.asar/main.js:111:28)
at breakComplete (/Applications/stretchly.app/Contents/Resources/app.asar/main.js:433:3)
at finishBreak (/Applications/stretchly.app/Contents/Resources/app.asar/main.js:447:3)
at Function.globalShortcut.register (/Applications/stretchly.app/Contents/Resources/app.asar/main.js:372:9)
Using latest version window popped up and I was using vscode to code some items when error showed up.

Hi and congratz on issue # 400! ;]
Do you use more monitors or one? Was it at the end of break?

Almost identical (except for finishBreak vs. postponeMicrobreak):

In my case (and I suspect it's the same in yours), it arises only when I click "Reset breaks" from the tray menu while a stretchly window is open. I've found that it's because stretchly doesn't release the CommandOrControl+X global shortcut when windows are closed this way. I've tested a fix here. Preparing a PR.

(Sorry, to clarify: the error appears when I use Ctrl+X in other applications after using "Reset breaks" as above. "Cut" operations don't work and after a couple of attempts, the error appears.)


TermsAggregatorTests flaky test fix

Description
Adding documents in a single chunk in unit test to avoid multiple segments in the unit test.
The following assertion was failing:
                    assertEquals(expectedCollectCount, aggregator.getCollectCount().get());

The number of collect() calls is used to check whether the optimization has kicked in or not. The tests written with tight assertions assuminga single segment.
In local development, running this test in isolation, it is not feasible to get the test failing as it might be creating a single segment. However, in gradle CI (see linked issue) and running the test with other tests, it may happen that multiple segments are created (multiple commits after individual document addition), which is the only explanation why the collect() count assertion is failing case.
So inspired from https://github.com/opensearch-project/OpenSearch/blob/main/server/src/test/java/org/opensearch/search/aggregations/bucket/histogram/DateHistogramAggregatorTests.java#L1655-L1668 editing the test to add all documents in a single call to avoid committing changes to index and avoid multiple segments.
Did not add changelog as this is a flaky test fix.
Related Issues
Resolves https://github.com/opensearch-project/OpenSearch/issues/12640

Check List

[x] New functionality includes testing.

[x] All tests pass

[x] New functionality has been documented.

[x] New functionality has javadoc added

[x] Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
[x] Commits are signed per the DCO using --signoff
[x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)
[x] Public documentation issue/PR created

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.

Looks like SpotLess check failed.  Can you take a look?

While you are at it, could also add some info about the validation you did for the change?

Adding documents in a single chunk in unit test to avoid multiple segments in the unit test.

Is the test dependent on the number of segments? Is that the assertion failing in the original bug?

If the test depends on a specific number of segments then can we [1] add an assertion for the segment count and [2] force merge to the correct number of segments within the test?
Not sure about the viability of [2] since I see a test path for deleted docs.

@jed326

If the test depends on a specific number of segments then can we [1] add an assertion for the segment count and [2] force merge to the correct number of segments within the test?

[1] No its not feasible to add assertion of segment count - I could not find a utility to do so in our codebase. Introducing such utility may not be within the scope of this change.
[2] Adding documents in a single call (addDocuments()) ensures commits are not happening in between multiple calls (addDocument()).
@jainankitk @kkmr
Added more details in description now.
Tests are not failing locally (most likely running in silo). But looking at failures, it looks like if we can force the documents are added in a single call, it will result in a single segment like the one in DateHistogramTest given we already have no merge policy in place during index creation. The failures in flaky tests reported in different CI calls can only occur when there are multiple segments so this is more of a calculated change that we can do to solve the flakiness with no side-effects.

The failures in flaky tests reported in different CI calls can only occur when there are multiple segments so this is more of a calculated change that we can do to solve the flakiness with no side-effects.

Can you try to run the test locally until failure with and without this change? And report back the number of successful runs without failure?

The failures in flaky tests reported in different CI calls can only occur when there are multiple segments so this is more of a calculated change that we can do to solve the flakiness with no side-effects.

Can you try to run the test locally until failure with and without this change? And report back the number of successful runs without failure?

So I have iterated through the tests 250 times in a loop for both scenarios (with & without my change) with 0 failures in each case. So still no luck reproducing it in present way.

So I have iterated through the test 250 times in a loop for both scenarios (with & without my change) with 0 failures in each case. So still no luck reproducing it in present way.

Can you explicitly issue IndexWriter.commit to create multiple segments and confirm the issue? Just want to ensure that this change is helping

Can you explicitly issue IndexWriter.commit to create multiple segments and confirm the issue? Just want to ensure that this change is helping

Hey, thanks for suggesting this. In the original code, I added indexWriter.commit(); after 2 addDocument() and I was able to fail the tests accordingly.
Meaning cases where there will 2 segments + deleted document case  and optimization will kick in for only of them and not both - test will fail with the same CI exception.
So this validates my suspect of multiple segments being the issue.

@sandeshkr419 - Thanks for confirming the issue. Approved!

No its not feasible to add assertion of segment count - I could not find a utility to do so in our codebase. Introducing such utility may not be within the scope of this change.

Since you have an IndexReader, could expectedCollectCount be a Function<IndexReader, Integer>, then for the case where the optimization kicks in, you can pass in a lambda like (r) -> r.leaves().size()? For the non-optimized case, it would be (r) -> docCount, I guess.

I think I missed where I can check the number of segments using reader.leaves.size()

Since you have an IndexReader, could expectedCollectCount be a Function<IndexReader, Integer>, then for the case where the optimization kicks in, you can pass in a lambda like (r) -> r.leaves().size()? For the non-optimized case, it would be (r) -> docCount, I guess.

The cases which are failing are when one of the segment can use optimized flow (where (r) -> 0) and another segment use the un-optimized flow (r) -> docCount.  I think I can assert expectedCollectCount to be number of LiveDocs in segments having deleted documents (unoptimized segments) as segments with 0 deletes will not contribute to in collect() calls.

@msfroh I see this is merged. Would you recommend reverting this back and have a multiple segment case as we discussed offline.
Tbh, I like it in present state since its much more atomic as the improvements were on segment level and we are forcing the index creation with a single segment. If required, we can have a multiple segment case separately independent of the tests in this PR.

As per offline discussion with @msfroh - will keep this change and will add a separate test to cover multi-segment scenario.


Author class IAO_0000442

From<EMAIL_ADDRESS>on October 30, 2011 18:48:10
The annotation IAO_0000115 says that an author is:  "a role inhering in a person or organization that is realized when the bearer participates in the work which is the basis of the document, in the writing of the document, and signs it with their name."
Comments/questions/suggestions:
This is really a relation, right? Aaw, a = an agent responsible for creating the work, w, the work, the relation "a authors w." Sometimes we do say that someone is an author without saying what it is was authored, but that doesn't seem to be the kind of use that will be made of it here. Not recognizing that this is a relation explains the awkward use of "the document" in the annotation quoted above.
I suggest replacing "author" with something more general, such as the Dublin Core "creator:" "An entity primarily responsible for making the resource." ( http://dublincore.org/documents/dcmi-terms/#terms-creator )
This accounts for more than just authors of text documents, for instance, illustrators, photographers, a programmer or someone else that creates some kind of visualization, a cartographer.
Also---the person signing the document is not necessarily its creator. Think of a PI putting his or her name first (or last) on a publication, even though he or she advised a post-doc on the work but did not actually do much of it. MARC/ISBD uses "statement of responsibility," which is supposed to be read directly from the chief source of information about the work, eg., the title page, in a book. This may be different than the creator, because this is supposed to be read from the work directly. Perhaps there should be a separate set of classes for responsibility and for creator?
Original issue: http://code.google.com/p/information-artifact-ontology/issues/detail?id=127

From<EMAIL_ADDRESS>on October 30, 2011 15:56:22
DC has the awkward reference to "the document" as well---DC is intended for use in a bibliographic record, in which the work is identifiable immediately as the work that the bibliographic record is about, so this fixes reference to "the document." This is not necessarily so in the case of an ontology. I might have a work, say, the Origin of Species, and identify it by some CRID, and then relate it to people such as hasCreator(Darwin) or hasPublisher(John Murray).


[Exteral Request] - Table Item Ledger Entry - GetCurrencyCode

Could this be made global?
 local procedure GetCurrencyCode(): Code[10]
    begin
        if not GLSetupRead then begin
            GLSetup.Get();
            GLSetupRead := true;
        end;
        exit(GLSetup."Additional Reporting Currency");
    end;

Thanks for reporting this. We agree, and we’ll publish a fix asap, either in an update for the current version or in the next major release. Please do not reply to this, as we do not monitor closed issues. If you have follow-up questions or requests, please create a new issue where you reference this one.
Build ID: 12340.


As hard to use as Bootstrap

Bootstrap is very low-level: all the CSS, JavaScript, and other pieces and the many limitations have to be understood in detail and written manually. But WSK is just as bad an experience for Windows users, as the instructions appear to assume the use of Linux and deep familiarity with Linux development environments.
I wouldn't make an issue of these facts if it weren't for the unqualified claims that are made in all the intro material. We Windows users just assume that everything will just work, and will not require Linux. It is rough when we download WSK and discover the truth.
I strongly suggest that all the intro documentation be edited to reveal that this is a Linux-only development structure.
I think the truth is that no really easy-to-use development environment for responsive Web design exists. Many days of Web searching has led me to this conclusion.

An additional limitation not disclosed up front: the instant editing feature requires installation of a specific editor (Sublime) for a charge of $70 (with free trial). Its description sounds nowhere as extensible as Emacs; the emphasis is on selling it on the basis of a few flashy features. Note that many editors, such as the free Arachnophilia, support instant Web editing.
This little bit of hidden moneymaking is only offensive because iof the glowing generalities of the intro descriptions of this package, described above in the issue description, which actually does not include Sublime.

Please include specifics.  What specific problems did you encounter?

Well I am using WSK in Windows 7 x64 with Netbeans and Eclipse and all i had to do was running the commands described in the readme.
Btw live reload is not a feature of sublime text.

An additional limitation not disclosed up front: the instant editing feature requires installation of a specific editor (Sublime) for a charge of $70 (with free trial).

This is only presented as an example. The developer is free to chose whichever editor they wish and if it supports live-reload then they can get that integrated just fine.

This little bit of hidden moneymaking is only offensive because if the glowing generalities of the intro descriptions of this package, described above in the issue description, which actually does not include Sublime.

Um, who is making the money if people buy a Sublime License? Not anyone working on this project. We push ST since it is a very capable editor that is cross-platform and easy to use. Vim and emacs, as you suggest as preference, are not nearly as easy to use. We can't recommend them for the beginners the documentation is built for. Instead of recommending three or four editors to cover different platforms, we pick one that works across them all nicely. This makes documentation and helping others along much easier.

I strongly suggest that all the intro documentation be edited to reveal that this is a Linux-only development structure.

As @sindresorhus requested, please provide specific examples. It sounds as if by "intro material" you are talking about Web Fundamentals, not WSK. WSK's intro doc should be straightforward enough to follow in any OS. Install NodeJS, npm should then be in your global path and you can run the other necessary commands.

I wouldn't make an issue of these facts if it weren't for the unqualified claims that are made in all the intro material.

What are the "unqualified claims"? Probably will refer to the previous request for specifics. We can't fix vague references. Providing specific pieces of content will help us understand what needs to be improved.

We are working to make things much better with version 0.6 by having an out-of-the-box experience that doesn't require any external software from npm. This has been planned at least for a few weeks now, covered in issue #660.

Using WSK with Win7 x64 and Win8.1 x64 with WebStorm and it works just fine (including live reload, gulp and jshintrc integration etc.).
Though I agree it can be quite a pain to setup, but that's mostly Ruby for Windows fault in my opinion (see this which I had to do, I needed to fully uninstall Cygwin and its own Ruby distribution so they don't conflict and also #649 was also bugging me back then and making things difficult).
Seems like #660 will make things quite a bit easier in general though.

@Flaiker I do believe we have done away with the Ruby dependency. I think it was only necessary for Sass, but we have since switched to lib-sass from node.

Actually WSK works good on Windows. I use a mac and my Dev team uses windows. You may not be able to run it on IIS but since it is a front-end framework just complete your HTMLs before Back-end gets their hands on the. Here are the instructions I gave my Windows Dev Team. Read it carefully as there are some issues when working with a team that uses both platforms (Mac/Windows).
Installing Git:
Download the latest Windows build Here:
http://git-scm.com/downloads
Start The installation....
Once you reach the 'Adjusting you PATH environment' section select the 'Run git and included Unix tools' from the Windows command prompt.
Next, under 'Configuring the line ending conversions' make sure 'Checkout Windows-style commit Unix-style line endings' is selected.
Should complete the install for git. In cmd type 'git --version' to confirm installation
Install Node and NPM
Download the latest windows build here:
https://nodejs.org/
Installation process is simple with no special instructions.
type 'node -v' to see version of node and confirm install
type 'npm -v' to see version of npm (installed with node) and confirm install
Install Ruby and  Gems (Required for SASS)
Download the Ruby 1.9.3 (for compatibility) build for windows here:
http://rubyinstaller.org/
Installation process is simple with no special instructions.
type 'ruby -v' to see version of ruby and confirm install
type 'gem -v' to see version of gem (installed with ruby) and confirm install
Installing Sass
type 'gem install sass' into your cmd (you may have to run as Admin)
type 'sass -v' to see version of sass and confirm install
Install GulpJS
In cmd type 'npm install -g gulp' if you get errors install as admin
type 'gulp -v' to verify
Install Bower
In cmd type 'npm install -g bower' if you get errors install as admin
type 'bower -v' to verify
Generating SSH Key (For working with REPOS and Server)
To generate an SSH key with PuTTYgen, follow these steps:

Open the PuTTYgen program.
For Type of key to generate, select SSH-2 RSA.
Click the Generate button.
Move your mouse in the area below the progress bar. ...
Type a passphrase in the Key passphrase field. ...
Click the Save private key button to save the private key.

I apologize for the too-brief list of complaints that I gave here. My goal was to write a modern website (I'm a retired web designer and software engineer), and with this goal I encountered nothing but frustration in looking at what has been done to help designers. I learned that something like Bootstrap would be needed, at least.
As for WSK, I'm afraid that, with my background being mostly in Windows, I have no tolerance for working with a maze of new libraries, programming languages, and technologies, which apparently are enjoyed by those who use Linux every day. I want to use what seem like simple basics to me: PHP, Javascript, HTML, and CSS.
After about a week of intense research, including trying out Froont, which could end up being usable someday, my initial feeling that Responsive Design is still in its infancy seems well confirmed.
FWIW, I chose a methodology that allowed me to make fast progress in coding a new website: I created my own framework in PHP that localizes each part of the visible website and each internal feature, using ordinary PHP functions, for ease of debugging and extending. The actual HTML, CSS, and Javascript that are generated are mostly what are required to support each Bootstrap feature, and most of the development time is spent in figuring out what the Bootstrap documentation fails to say (which in the case of the basic Affix feature, when embedded in a real website,  is immense). Development is going so quickly that I have been able to design a great unlimited and easy to use language translation system to help our volunteers translate the website and maintain the translations easily.
I suspect that someone with more free time and expertise than myself will someday create a really universal framework for designing and implementing websites easily. It is sorely needed.

I'm sorry you've had a hard time getting this to work, but there's plenty of documentation and @fsquaredmedia has even wrote down the exact instructions for installing all the dependencies you'd need.
And you haven't really described what technical problems you're having. It seems like you have the wrong expectations about this. You still require some sort of web development knowledge to create an app with this.

https://github.com/google/web-starter-kit/issues/660 might help.


New version: LazyBandedMatrices v0.7.7

Registering package: LazyBandedMatrices
Repository: https://github.com/JuliaMatrices/LazyBandedMatrices.jl
Created by: @dlfivefifty
Version: v0.7.7
Commit: c8ee075ed8491a4a6fc5eca2d2ec569961027849
Reviewed by: @dlfivefifty
Reference: https://github.com/JuliaMatrices/LazyBandedMatrices.jl/commit/c8ee075ed8491a4a6fc5eca2d2ec569961027849#commitcomment-69334919
Description: A Julia package for lazy banded matrices

block

[noblock]


PEP8 compliance in pydot.py

Hi @hrishikeshrt, thanks for this PR. The timing is great, at the start of a new development cycle.
I think we should be able to merge this soon. There is only one thing I really have strong doubts about:

These separator lines:
# --------------------------------------------------------------------------- #

Is there any style guide or style checker that recommends this practice or is it just a personal preference? I can see their use, but personally I doubt I would depend much on them when moving through the code. More importantly, if there is no standard to enforce them, then with multiple contributors working on the code I am afraid their usage will become inconsistent over time. I also do not want to introduce a project-specific style rule for this. So unless there is a style guide or linter that this came from that you recommend, I would prefer to leave them out.

My other remarks are optional. You can decide per item what you want to do with it. They are not blocking merging as far as I am concerned:

From the remaining flake8 complaints: pydot.py:110:80: E501 line too long (82 > 79 characters). Any reason not to add a line break here? Maybe the 10% rule used by Black and Bugbear B950?
From the remaining flake8 complaints: pydot.py:393:5: F841 local variable 'node_orig' is assigned to but never used. That variable was probably left over after copy-pasting from the method above it. If you want, you can remove it.
Commented-out code # for attr in self.obj_dict['attributes']: can actually be deleted. It was introduced by commit pydot/pydot@842173ca of 2008 (pydot 1.0.2) and it looks like it was an early draft still based on the idea that obj_dict['attributes'] would contain all possible Graphviz attributes, just as the attributes property did before pydot 1.0.2. However, in the end the lists of Graphviz attributes were moved to constants to be passed to obj_attributes. Anyway, I do not think anybody still needs that comment. If you want, you can delete it.
Commented-out code # self.node_dict[graph_node.get_name()] = graph_node.attributes can be deleted as well. It was also introduced by commit pydot/pydot@842173ca of 2008 (pydot 1.0.2). The commented-out line is clearly outdated, because I do not see when node_dict was ever in use and .attributes was removed in that same commit. It seems more like a very early draft for the line above it than a hint or alternative for future readers.
This commented-out code can also be deleted:#match.extend( Subgraph( obj_dict = obj_d )
#             for obj_d in obj_dict_list )

Introduced by commit pydot/pydot@b125c5d7 of 2010 (pydot 1.0.3). Also seems to have been an early draft. Not sure if it would even work.
test/pydot_unittest.py not done yet. Not meaning to ask you to spend more time than you have. Your work is already greatly appreciated.
In the end I would prefer to squash everything into one single commit. If you want, you can do that already and force-push over this PR branch. I can also do it on merging.

Side notes, not requiring change to your proposal:

AppVeyor CI failure not related to the changes of this PR, but due to known issue pydot/pydot#213. Travis CI passes.

From the remaining flake8 complaints:
pydot.py:32:16: F821 undefined name 'basestring'
pydot.py:308:13: F821 undefined name 'unicode'
pydot.py:1688:21: F821 undefined name 'unicode'
dot_parser.py:34:16: F821 undefined name 'basestring'

Related to Python 2 and safely behind a conditional checking for PY3. Indeed keep them for now as proposed. Will be removed later, after pydot/pydot#229.

From the remaining flake8 complaints:
dot_parser.py:407:9: F841 local variable 'dot' is assigned to but never used
dot_parser.py:408:9: F841 local variable 'slash' is assigned to but never used
dot_parser.py:409:9: F841 local variable 'bslash' is assigned to but never used
dot_parser.py:410:9: F841 local variable 'star' is assigned to but never used

These unused local variables are part of a longer list of variables, most of which actually do get used. Indeed, it is probably better to just leave the few unused ones around as well, in case we ever need them in the future. So, no need for changes to your proposal.

In graph_definition() and quote_if_necessary(), some vertical alignment gets lost, making them slightly less readable. But indeed, PEP 8 Pet Peeves recommends avoiding this. Pros and cons can be found here:

https://stackoverflow.com/questions/13497793/python-alignment-of-assignments-style
https://stackoverflow.com/questions/4001375/is-it-considered-a-good-style-to-use-whitespace-to-align-the-code

I have no problem with giving up the alignment here, mainly because readability is not hurt too much and it prevents complaints from code checkers, which is also burden. Again, your proposed changes here are ok for me.

The commented-out code # dblQuotedString and # .setParseAction(strip_quotes) | are also very old, from commit pydot/pydot@842173ca of 2008 (pydot 1.0.2). But in this case, we actually might be working on that part of the code soon, so maybe they will come in handy then and can be deleted after that. So, your proposed change is ok as it is.

Last year, @ankostis suggested letting Black take care of coding style. I tried it out yesterday and it does seem quite easy, though also very opinionated. The initial run will make many changes to our code: Not only PEP 8, but also many other things (longer maximum line length, additional line breaks, additional parentheses, double quotes everywhere, etc.). So, I think we will need some more time to let everyone review the changes brought in by Black. I am thinking of merging your PEP 8 changes soon already and then open a separate PR for the introduction of Black. This has the added benefit of seeing only the non-PEP8 changes that Black introduces. Love to hear your opinion on Black (or alternatives). And if you are interested in preparing that PR as well, please let me know beforehand, so that we are not doing double work.

Yes, the separator lines are a personal preference and a force of habit. I will get rid of those.
The reason I did not break line 110 was because it had  != [] at the end and breaking it was really hampering the readability of the code. I've updated this by splitting it in two lines
Got rid of node_orig = 1 unused variable
Got rid of several comments that you have mentioned
Completed pydot_unittest.py as well (with exception of two unused graph instances, which I suspected might be part of the test case to not raise error.
Regarding Black, I have no personal experience, and if it means going away from PEP8, then that's a negative for me. However, I will need to check that out before giving an informed opinion though. (I am open to submitting that PR as well if we end up deciding on using Black).

Have you considered black instead of/in addition to flake?

I would also suggest to just use black (+ integrate it in CI) and forget about formatting ;)

@hrishikeshrt Thanks for all the changes. A few more remarks:

Yes, the separator lines are a personal preference and a force of habit. I will get rid of those.

Ok, thanks, but now there are still 7 left in pydot.py. They have only been removed from dot_parser.py.

The reason I did not break line 110 was because it had  != [] at the end and breaking it was really hampering the readability of the code. I've updated this by splitting it in two lines

Ok, yes, I see what you mean. Nicely solved.

For the unused variables which may be used later, I've added #noqa to suppress the current complaints.

Hmm. Not really necessary for me actually. Some considerations:

Without a specific error code, # noqa will hide other problems as well. Coincidentally, there is now a E261 error hiding in line 397 already (E261 at least two spaces before inline comment). So # noqa: F841 (and an extra space) would be better, but...
Ignoring it in pylint as well would require another pragma again, and the pylint one needs to come first: # pylint: disable=unused-variable # noqa: F841. That makes it even longer, so...
It would be nice if we could apply the rules to the entire block instead of just those 4 lines, but ignoring a block of lines is not possible in flake8 right now. (It is possible in pylint by toggling the rule using # pylint: disable=unused-variable, # pylint: enable=unused-variable.)
More importantly, we do not have any other pragmas for flake8 or pylint in the code right now and I am still not sure if we should start that practice at all. I have been considering a pylint pragma as part of PR pydot/pydot#242, but also still not sure about it. They could make the code quite messy in the long term while perhaps not so many people actually run these tools on our code.

What do you think? If it is all the same to you, maybe better remove them for now.

Completed pydot_unittest.py as well (with exception of two unused graph instances, which I suspected might be part of the test case to not raise error.

I see flake8 report only one (not two) unused graph instance, in line 44, in test_add_style(). Although I think it can be removed, I agree that such a change does not really belong in a PEP8 commit. So, I agree that it is better to leave that line in for now, as you propose.
The second issue I see flake8 report is about a missing space:
pydot_unittest.py:65:12: E225 missing whitespace around operator

From line 65:
d ='digraph {\na -> b[label="hi", decorate];\n}'

Hope you can still correct that.
For the rest, all your changes to test/pydot_unittest.py look good to me.
One new remark, something that I did not notice last time, sorry. Optional:

I see you that in many places you remove white space between the def lines and the first lines of code, but not everywhere. A clear example is here: Still an empty line below def push_graph_stmt(), while the one below def push_subgraph_stmt() was removed. Also in a few other places in dot_parser.py and test/pydot_unittest.py.

Separate comment on black coming up tomorrow, but that should not affect this PR.

Removed the leftover separator lines in pydot.py, sorry about missing them earlier
Removed # noqa
I actually missed the space around operator error and thought it's an error about graph declaration on the next line (that's also why I mentioned there are two unused graph variables). Corrected.
I've removed the blank lines after def. The ones left over were not intentional but from an oversight.

Thank you.

Wow, you're faster than I can turn off my computer. :smile:
But you still missed a few blank lines after def lines, including the one I mentioned. Anyway, like I said this is optional, so you decide if you still want to make a final correction or not and then I want to move on to merging.
Do you want to squash your commits yourself or shall I do it on merging?

I think I've got them all this time :sweat_smile:
I've also squashed commits (I believe).
(In my repository I only see a single commit, although all of them show up here, so I am not sure if it was done properly.)

Yes, but now you also removed empty lines below a few class lines (or their docstring). Those actually should have an empty line below them. See https://stackoverflow.com/questions/18914108/python-pep-blank-line-after-function-definition/34818759#34818759 and https://www.python.org/dev/peps/pep-0257/.
Also, you removed empty lines in other parts of the code, while this was supposed to be a final correction.
Could you please just revert from the last commit before squash (pydot/pydot@17f4a35) all changes that went beyond what I requested in my last comment (only remove the blank lines after def)?

* This has the added benefit of seeing only the non-PEP8 changes that Black introduces. Love to hear your opinion on Black (or alternatives).

Yes, indeed.
But you'll see, once black, never back :-)

@ankostis Yes, maybe you are right, but I also still have a few questions/doubts about black. Will try to post them here soon.

Regarding the longer line-width, there been much debate, sprung from Hettinger's "don't PEP8-me" notorious presentation, and it does make sense in multiple ways, e.g. we don't want a slight change to produce a much bigger git-diff.
In general black has been streamlined for git and cooperating developers, mimicking the success of Golang's automated formatting. The whole point is to avoid having lengthy discussions like the one above.

Could you please just revert from the last commit before squash (17f4a35) all changes that went beyond what I requested in my last comment (only remove the blank lines after def)?

Done. Also, There were some instances where class instances did not have a blank line after them. Added that as well.
Please take a look.
I have not squashed again. If this version is okay, you may squash post merging (unless you want me to squash).

@hrishikeshrt
Yes, class headers look much better now, thank you for picking that up as well.
Empty lines below def not completely consistent yet. Your last correction even brought back some of the empty lines you had correctly removed in pydot/pydot@17f4a35, but compared to master most are gone now and no new ones were introduced, so in total a huge improvement.
Overall, a lot of inconsistencies removed, completely PEP 8 compliant now and everything looking great again. So, finally merged. Thanks a lot for all your hard work!
I will probably need another day to write down my thoughts on black.

@ankostis wrote:

Have you considered black instead of/in addition to flake?
[...] you'll see, once black, never back :-)

@prmtl wrote:

I would also suggest to just use black (+ integrate it in CI) and forget about formatting ;)

As you may have noticed from my delayed reply, I have been struggling with this for days now already. Especially because the two of you are so positive about it, while I am not overly enthusiastic yet. I can see the promise of automated code formatting, but I do not know if it is going to be so enjoyable to work under it. It takes away part of the art, the fun. There is no leniency at all. If the result is ugly, just suffer it. Computer says no. Another part of our lives controlled by a bot.
And not even a very good bot, I feel. I have also been looking at yapf and it seems a bit smarter, or at least it aspires to be, with its penalty scoring approach. Also, it seems the yapf developers are more open to improving their tool to produce the most optimal code formatting for humans to work with, aware that that bar is very high and has not been reached yet. With black I get the impression that they fixed the bar a few years ago and now the goal is just to keep producing the same result forever. I looked up some of the issues I had with black on their issue tracker and the answer always seems to be like: "No, sorry. Remember our slogan: Uncompromising.". But while I can understand you want a code formatter to be uncompromising at the moment you run a certain version of it, I don't need it to be uncompromising between versions. They could at least try to improve over time.
A non-technical problem I have with yapf though is that it is led by Google, meaning a Contributor License Agreement (CLA) for contributors and uncertainty about its future in case Google suddenly loses interest in it. In contrast, black is under the Python Software Foundation umbrella.
Then there is autopep8, which I also tried out a bit. Given that we just had a PEP8 refresh, it does not make many changes. It has a much more limited reach than black and yapf, meaning it leaves a lot of formatting untouched. But in the areas that it covers, the choices it makes are the same as the choices black and yapf make. So, it could be used as a first step into automated code formatting. A problem with autopep8 though is that it is a much smaller project and that it needs some strange command line options like -aa --experimental (-a meaning --aggressive). An issue to make those flags the default has been open for a few years already. Also, I do not know how well it can integrate with CI.
@ankostis wrote:

Regarding the longer line-width, there been much debate, sprung from Hettinger's "don't PEP8-me" notorious presentation, and it does make sense in multiple ways, e.g. we don't want a slight change to produce a much bigger git-diff.

Yeah, ok, well, I watched that Raymond Hettinger talk, but I am not sure if black catches the full meaning of it. Ok, it raises the line-width from 79 to 88, but in enforcing that black seems to be just as strict as someone who strictly enforces the 79 limit of PEP 8. The documentation mentions the 90-ish characters, the 10% tolerance, so my first impression was that 88 was the result of a 80 character limit plus 10% tolerance, that sounded reasonable. But in reality, 88 seems to be just the new limit and there is no tolerance. If you have a multi-line collection of items and try to stay within 80 characters, because you do not want to use that 10% tolerance there, black will still just reflow your code to fill up lines to the maximum of 88. And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"? Maybe I am missing it, so please correct me if I am wrong. Otherwise it seems black simply applies its rules just like a PEP 8 zealot applies PEP 8: Like a law book. Which is exactly what Raymond Hettinger in that talk says not to do.

In general black has been streamlined for git and cooperating developers, mimicking the success of Golang's automated formatting. The whole point is to avoid having lengthy discussions like the one above.

Yes, it is easy to point out a lengthy discussion about code formatting style, but I think we do not have a lot of them actually. The last PEP8 refresh was in 2016. What is more difficult to point out is the costs of using a code formatter: How do you measure the cost of everyone having to scroll through 170 additional lines of code at the top of pydot.py, the cost of everyone having to ignore inconsistent formatting of similar lines because of slightly different line lengths, etc.
Also, how about newcomers? Is this not a slight entry barrier? Predicting a formatting tool is even more difficult than following PEP 8. So we would need to ask new contributors to install that tool (maybe even pip still), or else let them hit a failed CI check when they submit their PR. Does the CI check use the black --diff option so they can see what they are supposed to change? I assume we do not want a CI bot to make formatting commits by itself. I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet. For example, contributor submits PR, check fails, maybe some discussion, second try, check succeeds, PR gets merged. If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.
There is so much more to say, but I will leave it at this now. Hope to hear your thoughts. I am not all negative about it, just struggling with it.
Some comparisons:

master vs. black
master vs. yapf-facebook
master vs. yapf-google
master vs. yapf-pep8
master vs. autopep8
black vs. yapf-facebook

(I tweaked the settings a bit to make them easier to compare. For details, see the individual commits of each PR.)

I agree that black is not a tool that fits all. But actually this discussion would not happen if we would have black (or any opinionated formatter in that matter) ;) The point is - use it and forget, do not think about style anymore, do not think if better to use facebook style, google style or whatever. It will be always the same for everyone and (virtually) forever thanks to:

"No, sorry. Remember our slogan: Uncompromising."

Also, how about newcomers? Is this not a slight entry barrier?
This is just like with regular tests, if they fail - it need to be fixed. Bugfix? Please provide example.

And how it works now with PEP8? They need to (should) follow it as to not reject contribution due to formatting or badly formatted code will be introduced into the codebase. Even more: if they fail to stick to formatting rules, it's easier to just run black command by maintainer of pydot on the proposed change than to fix formatting manually.
For me it could be any formatter as long as you don't have to think when using it. Sadly with yapf you sometimes do, to just choose the options or preset.
IMHO lets not overthink it (as pydot is rather small codebase) and just integrate black on defaults or even yapf with one of the predefined config and stop worrying about formatting.

And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"?

As explained, the limit is 80 +8(%10), but in that style-manual they also suggest you to make it 90-ish, which btw, the line-length is one of the few configurable parameters of black.
Also you can always mark parts of the code to be excluded from the black, with #fmt: on/off,

Also, how about newcomers? Is this not a slight entry barrier?

Quite the contrary, black is a gift - to me it's always a delight to see the black-badge at the top of a project's coordinates:

As long as you have ignored any filename-regexes that must no autoformat, with a section in pyproject.toml like this:
[tool.black]
exclude = <regex>

all a contributor has to do it to type black . to format his code.

So we would need to ask new contributors to install that tool (maybe even pip still), ...

Actually it is a good habit to provide an extras [dev] in setup.py (or/and [all]) so all such dependencies are installed:
    extras_require={
        "dev": "black==21.5b2; python_version > '3.5'",
        ...

Note that the pinning is good for 2 reasons (1, to be stable, 2. it has no stable release, and causes dependency problem sometimes)

I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet.

I have contributed to programs that have a pre-commit hook for black`](https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/), but the experience wasn't that great.

If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.

Eventually having a test-case that simply runs black --check . or black --diff . demanding an ok exit-code, would print the user all unformatted files.
Look also the github-actions example.
I thought i had wrote something similar in travis, but cannot find it now.
The eventual experience as a contributor is rather amusing, you just curse yourself for not having run the test-cases locally prior to push, fix, push-force to re-write commit, and you get accustomed in no time.

Thank you for your always dutiful collection of resources, for the list of comparisons.
But I'm surprised you didn't already mention the reformatting of string-quotes to " - that is a first big surprise to newcomers to black.  It's only later that you come to appreciate its significant side-effect: you can immediately tell if some code has been black or not (yet).
In any case, as @ prmtl pointed out, the point is to have an opinionated auto-formatter, whatever that might be.

@prmtl @ankostis If we were to give black a try, would the following 4 choices be ok for both of you? These would solve my biggest gripes:

line-length = 79: A personal preference, because I often work on a small laptop or even my phone. And when I work on an external monitor, I often do side-by-side diffs and 3-way diffs.
skip-string-normalization = true: This prevents conversion of single quotes to double quotes. In case a better tool comes around in a few years that does not mandate double quotes. We can always switch to double quotes later if it really becomes the norm. Not a deal breaker for me, though.
Wrap the *_ATTRIBUTES constants at the top of pydot.py in the # fmt: off/on pragmas suggested by @ankostis to prevent they get exploded to a single item per line. I would rather have a larger diff of 60 lines once every few years than having 170 additional lines of code around all the time.*
No pressure on contributors to install the formatting tool locally or to ensure their code passes the formatter before they submit a PR. This affects how we document it (a badge is ok of course), but also for example a formatting check in the test suite should not fail if the formatter tool (e.g. black) is not installed. I would not want to miss someone sharing a quick bug fix. The formatter check can be part of the CI checks on GitHub after the PR is submitted, preferably listed separate from the regular test suite**. In case of formatting issues, we can always suggest they fix them, but if that is not possible (e.g. we responded too late and the contributor has moved on to do other things), we can also just merge their work and fix the formatting ourselves in a subsequent commit.

* I guess the exploding of these long lists is what got black and me off on the wrong foot: It was the first thing I saw after running black for the first time. I really had to get past that to discover that the rest was not so bad. There is some discussion on this behavior (psf/black#1345, psf/black#1824, etc.), btw, but considering discussions like psf/black#1288 I am not getting my hopes up.
** Also the formatting check needs run only once, no need for running it on multiple architectures and Python versions.
With regard to yapf, I went through the comparison of black vs. yapf (facebook style) of peternowee/pydot#7 and I cannot say one is clearly better than the other. In some areas, yapf can be told to behave like black by adding trailing commas or through configuration, but some things are simply not possible in yapf yet, such as adding/removing parentheses or listing multiple logical operators line by line. Forcing ourselves to stay with one of the predefined styles we would lose out on one of the advantages of yapf actually, but on the other hand I agree the time needed to agree on it all would simply not be worth it. So, no clear winner and if you guys prefer black, I am fine with that.
With regard to autopep8: If the premise is that less formatting/style choices is better, I guess the wider reach of black and yapf should be preferred over the limited reach of autopep8.
In reply to @ankostis

And if you need that 1 extra character and use 89, Black will reformat to 88 or lower again. So where is that tolerance, where is that "...-ish"?

As explained, the limit is 80 +8(%10), but in that style-manual they also suggest you to make it 90-ish, which btw, the line-length is one of the few configurable parameters of black.

The link is not working and I am not sure if you understood what I meant, namely that black (let's assume the default of 88 characters) does not treat the "+10% tolerance zone" (between 80 and 88) any differently than the "100%" zone (between 0 and 80 characters). AFAICT, it does not feel any restriction to use the full 88 characters when reflowing. And it is also not like 88 has become the new 100% with a 10% tolerance on top of that, because anything above 88 is treated as a violation. But I think the documentation already confirms my suspicion by using the word "happens" here:

Black defaults to 88 characters per line, which happens to be 10% over 80.
-- https://github.com/psf/black/blob/21.5b2/docs/the_black_code_style/current_style.md#line-length

I think a true implementation of a tolerance of 10% would be if the tool would only seldomly reflow lines to end inside that tolerance zone. yapf has an option split_penalty_excess_character that seems to be more like what I meant, although I did not have a chance to experiment with it yet.
Note that this is something else than the tool being "uncompromising" or not. A tool can have sophisticated rules on how to behave in the 10% tolerance zone and still enforce them in an uncompromising way.

Also, how about newcomers? Is this not a slight entry barrier?

Quite the contrary, black is a gift - to me it's always a delight to see the black-badge at the top of a project's coordinates:

Yes, ok, to you it is a gift, because you already know the tool and have it installed. I was thinking more about people sharing one of their first Python bug fixes ever.
Anyway, as I stated above, I am ok with it now as long as we do not force contributors to use it before submitting their work.

As long as you have ignored any filename-regexes that must no autoformat, with a section in pyproject.toml like this:
[tool.black]
exclude = <regex>

all a contributor has to do it to type black . to format his code.

Yep, that config file will need to be added. Not seeing any files that need to be excluded at the moment, but the other options I mentioned above go in that same file.

So we would need to ask new contributors to install that tool (maybe even pip still), ...

Actually it is a good habit to provide an extras [dev] in setup.py (or/and [all]) so all such dependencies are installed:
    extras_require={
        "dev": "black==21.5b2; python_version > '3.5'",
        ...

Note that the pinning is good for 2 reasons (1, to be stable, 2. it has no stable release, and causes dependency problem sometimes)

Yes, I guess version pinning is good to ensure the same outcome locally as on CI, but does this not lead to conflicts with other packages pinning on different versions? Or are these requirements installed inside the project directory? I do not have much experience with installing through setup.py.
Also, would it not give us the extra work of watching black updates and manually updating the pinned version regularly/in case of important bug fixes?

I tried to find some examples of projects using black CI integration to see how this works out in reality, but did not find any yet.

I have contributed to programs that have a pre-commit hook for black`](https://ljvmiranda921.github.io/notebook/2018/06/21/precommits-using-black-and-flake8/), but the experience wasn't that great.

If someone has some examples like that, please let me know, so I can get a feel for how this works in practice.

Eventually having a test-case that simply runs black --check . or black --diff . demanding an ok exit-code, would print the user all unformatted files.
Look also the github-actions example.
I thought i had wrote something similar in travis, but cannot find it now.

Thanks, I will check out the documentation. If you still find something later, please share. I can also search a bit more myself. Just a random search on GitHub already yields many results, but it seems like every project has a different approach and many of them are too small to have any example PRs I can look at. Anyway, the CI integration can be done separate from the reformatting and documentation, so I am not too worried about that.

The eventual experience as a contributor is rather amusing, you just curse yourself for not having run the test-cases locally prior to push, fix, push-force to re-write commit, and you get accustomed in no time.

Ok, cool. Thanks for taking away that worry.

But I'm surprised you didn't already mention the reformatting of string-quotes to " - that is a first big surprise to newcomers to black. It's only later that you come to appreciate its significant side-effect: you can immediately tell if some code has been black or not (yet).

Yes, it was one of the main things I did not like, but like I said "there is so much to say". I had been struggling with the whole idea of even handing out control to any formatting tool, and got blocked by the overchoice of tools and settings, all new to me. Time was ticking away, so I decided it was better to give a short response rather than none at all. The double quotes were not on the top of my mind that day. As written above, I do hope that we can start with not reformatting the quotes, but it is not a deal breaker for me.

In any case, as @ prmtl pointed out, the point is to have an opinionated auto-formatter, whatever that might be.

Ok, your messages are clear and understood. I am starting to get interested in trying it out. Hope you can agree with the proposal at the top of this comment.
In reply to @prmtl

Also, how about newcomers? Is this not a slight entry barrier?

This is just like with regular tests, if they fail - it need to be fixed. Bugfix? Please provide example.
And how it works now with PEP8? They need to (should) follow it as to not reject contribution due to formatting or badly formatted code will be introduced into the codebase. Even more: if they fail to stick to formatting rules, it's easier to just run black command by maintainer of pydot on the proposed change than to fix formatting manually.

Yeah, ok, but the difference is that now the humans can still make an exception. For example, it sometimes happens that some code blocks are similar to each other, many examples of that in pydot_unittest.py. By formatting them the same, it is easy to spot where they differ, so what they test. The formatter tool will just look at line length and reflow a few of these blocks and others not, so it becomes much more difficult to spot the similarities and the differences between the blocks. We cannot make any exceptions, because the tool will just keep reverting it. And we also would not want to use pragmas in too many places, because it creates a mess and also excludes the ignored blocks from other formatting.
Anyway, I have come to accept this as a trade-off and as said above, I am willing to try it out now. I do however hope you can accept my proposal not to use the defaults, especially for line length. Please still let me know what you think of my proposal at the top of this comment.
In reply to @hrishikeshrt

Regarding Black, I have no personal experience, and if it means going away from PEP8, then that's a negative for me.

No, I think it still adheres to PEP8. Maximum line length is actually configurable and PEP8 allows maximum line lengths up to 99 characters in case the team agrees on it. If I run black with line length 79 and then flake8, I only get one additional violation:
pydot.py:985:43: E203 whitespace before ':'

For this line:
b = node_str[node_port_idx + 1 :]

Actually this seems to be a problem with flake8 and pycodestyle rather than with black. See:

https://github.com/psf/black/blob/21.5b2/docs/the_black_code_style/current_style.md#slices
https://github.com/PyCQA/flake8/issues/257
https://github.com/PyCQA/pycodestyle/issues/373
https://github.com/PyCQA/pycodestyle/pull/914

So, using black would not mean going away from PEP8. It just makes a lot of other changes that go beyond the scope of PEP8.

However, I will need to check that out before giving an informed opinion though. (I am open to submitting that PR as well if we end up deciding on using Black).

I do not know if you have been working on this more? Any thoughts on the whole discussion? I have gotten to know black a bit now, so I could also create a PR myself probably, but due to personal circumstances I am not sure if I can see it through to the end. If you do it, I would still have a lot of suggestions based on my experiments of the last few days, so that would not make it much easier, but maybe the end result will be better with two pairs of eyes on it. Let me know what you prefer.

I've been passively following the discussion. I can see the appeal in "never having to format the code" ourselves, sure, but I am not entirely in agreement of "write code and forget about formatting". To elaborate, if one writes code in a certain way (which is not horrible, say mostly PEP8 compliant), Black changes it according to its opinions and one comes back to write something again and doesn't see "familiar" code, it can be a bit off-putting. We can only "forget" about formatting to an extent, since the whole point of formatting is make it "comfortable" for humans.
That said, I think Black does a decent job (anecdotal evidence, as per some small to moderately large examples I've tried in their online playground). I also prefer the line limit of 79 (or 80).
You may create PR yourself. I'll follow the progress and make comments if I have any. (Unfortunately, I cannot commit to the task at the moment).
PS: Thank you for your detailed review of the pull request from earlier and overall thorough research. I certainly enjoyed the experience.

Continued in https://github.com/pydot/pydot/pull/267 with a PR for Black.

(i know the issue if line-width has been resolved, but found it relevant whatsoever for any future discussion)

Linus pushes for wider-than-80-char lines for tools like grep, etc.
DHH (RubyOnRails founder) also pushes for that.


java.lang.ArrayStoreException: source of type IOSByteArray is not an array

We are experiencing a strange crash due to an uncaught Java exception in an application which includes both code transpiled with J2ObjC AND an external framework transpiled with J2ObjC. The local code and the library have been transpiled with the same version of J2ObjC, which is version 2.7.
The exception is thrown at the beginning of a custom thread started by the external library. Being in the library, we have no access to the original Java code. What we know for sure is that the same library used in the same application, but excluding the local transpiled code and J2ObjC static libraries, works correctly and throws no exception.
The exception stack trace is as follows:
2021-12-28 17:42:39.313040+0100 ######[34201:1972018] *** Terminating app due to uncaught exception 'JavaLangArrayStoreException', reason: 'java.lang.ArrayStoreException: source of type IOSByteArray is not an array'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff20422fba __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007fff20193ff5 objc_exception_throw + 48
	2   ########################            0x00000001125fc6b7 +[JavaLangSystem nanoTime] + 0
	3   ########################            0x00000001125a446d -[JavaIoByteArrayInputStream readWithByteArray:withInt:withInt:] + 141
	4   ########################            0x00000001125a15e3 -[JavaIoBufferedInputStream readWithByteArray:withInt:withInt:] + 150
	5   ########################            0x00000001126fe789 -[JavaUtilProperties_LineReader readLine] + 178
	6   ########################            0x00000001126fbf8b JavaUtilProperties_loadLineReaderWithJavaUtilProperties_LineReader_withJavaUtilProperties_KeyValueLoader_ + 102
	7   ########################            0x0000000112793d84 -[JavaUtilLoggingLogManager readConfigurationWithJavaIoInputStream:] + 151
	8   ########################            0x0000000112793600 -[JavaUtilLoggingLogManager readConfiguration] + 532
	9   ########################            0x0000000112792a83 JavaUtilLoggingLogManager_checkConfiguration + 34
	10  ########################            0x0000000112792a1b JavaUtilLoggingLogManager_readPrimordialConfiguration + 65
	11  ########################            0x0000000112795c05 -[JavaUtilLoggingLogManager_1 run] + 94
	12  ########################            0x000000011279292a JavaUtilLoggingLogManager_ensureLogManagerInitialized + 226
	13  ########################            0x00000001127929ad JavaUtilLoggingLogManager_getLogManager + 31
	14  ########################            0x0000000112798da4 JavaUtilLoggingLogger_demandLoggerWithNSString_withNSString_withIOSClass_ + 39
	15  ########################            0x0000000112600d3e -[JavaLangThread_SystemUncaughtExceptionHandler uncaughtExceptionWithJavaLangThread:withJavaLangThrowable:] + 40
	16  ########################            0x00000001125fe05b start_routine + 127
	17  libsystem_pthread.dylib             0x00007fff6034a4f4 _pthread_start + 125
	18  libsystem_pthread.dylib             0x00007fff6034600f thread_start + 15
)
libc++abi: terminating with uncaught exception of type JavaLangArrayStoreException
*** Terminating app due to uncaught exception 'JavaLangArrayStoreException', reason: 'java.lang.ArrayStoreException: source of type IOSByteArray is not an array'

Any suggestion on what could be the cause or how to solve this?

After some more inspection, that stack trace seems to present a double exception, i.e. an exception thrown while handling an exception.
The original exception in unknown, but is caught and handled by this SystemUncaughtExceptionHandler, which is implemented in the jre_emul Thread.java source. This system exception handler tries to log the exception using java.logging.
While logging, a secondary exception of type java.lang.ArrayStoreException is thrown and this is what is shown in the stack trace.
So, it seems there's some kind of error in the SystemUncaughtExceptionHandler that at least prevents the original uncaught exception to be logged.

Ok, solved by myself.
The problem here is the external framework carries on all the static libraries from J2ObjC. So there's no need to add the typical build settings for J2ObjC in the project, i.e. Framework Search Path and Library Search Path must be left empty, while Header Search Path must still be set.
Once removed the settings for framework and library search paths, the project still built successfully and the crash disappeared.
Hope this may be of help to anyone incurring in the same error.

To be more precise:

IF your project makes use of J2ObjC transpiled code
AND has a dependency from an external library which in turn makes use of J2ObjC transpiled code
THEN set the project Build Settings as follows:

Other Linker Flags: leave empty, i.e. DON'T ADD -ljre_emul etc.
User-Defined Settings: ADD J2OBJC_HOME as specified in Xcode Build Rules
Framework Search Paths: leave empty, i.e. DON'T ADD ${J2OBJC_HOME}/frameworks
Library Search Paths: leave empty, i.e. DON'T ADD ${J2OBJC_HOME}/lib
Header Search Paths: ADD ${J2OBJC_HOME}/include as specified in Xcode Build Rules

The project will build successfully thanks to the J2ObjC includes on the search path, and will link successfully thanks to all the symbols already present in the external library.

Thanks for posting the solution to this. It looked like duplicate runtimes
were referenced, but from just looking at the stack trace it wasn’t obvious
how that happened.
On Wed, Dec 29, 2021 at 8:59 AM Gianluca B. @.***>
wrote:

To be more precise:

IF your project makes use of J2ObjC transpiled code
AND has a dependency from an external library which in turn makes
use of J2ObjC transpiled code
THEN set the project Build Settings as follows:

Other Linker Flags: leave empty, i.e. DON'T ADD -ljre_emul etc.
User-Defined Settings: ADD J2OBJC_HOME as specified in Xcode
Build Rules
https://developers.google.com/j2objc/guides/xcode-build-rules
Framework Search Paths: leave empty, i.e. DON'T ADD
${J2OBJC_HOME}/frameworks
Library Search Paths: leave empty, i.e. DON'T ADD
${J2OBJC_HOME}/lib
Header Search Paths: ADD ${J2OBJC_HOME}/include as specified in Xcode
Build Rules
https://developers.google.com/j2objc/guides/xcode-build-rules

The project will build successfully thanks to the J2ObjC includes on the
search path, and will link successfully thanks to all the symbols already
present in the external library.
—
Reply to this email directly, view it on GitHub
https://github.com/google/j2objc/issues/1827#issuecomment-1002607502,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAW2JL2CGLNONFMONTCHIGLUTMH3TANCNFSM5K4POBDA
.
Triage notifications on the go with GitHub Mobile for iOS
https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
or Android
https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you are subscribed to this thread.Message
ID: @.***>


Timeline / YouTube issue

If you choose to enable only the "Embed" uploader for the default module Boonex Videos, and you try to embed a youtube video from the timeline (i.e. in the homepage), you get the video informations and the thumbnail but...the button "Submit" has no effect. The video is not embedded/uploaded.
Browsing the folder "flash/modules/videos/files"
you can see here 3 files:
1_temp.jpg
1_temp_small.jpg
1_temp_small_2x.jpg

The upload/embedding fails but these (temp) files are still here (folder "flash/modules/videos/files") .
There is another issue related to this problem.
Later, all the youtube uploads displays always a wrong thumb (due to the presence of the temporary files created by the failed upload)

Confirmed on my test site.

on my site as well

http://www.boonex.com/forums/topic/Timeline-YouTube-issue.htm

Related issue:
http://www.boonex.com/forums/topic/Dolphin-7-2-Bugs.htm
point 1)


build(deps): bump chevrotain from 9.0.1 to 10.5.0

Hello @paustint,
I bumped chevrotain to the latest version too.

10.0.0
Dropped support for legacy ES5.1 runtimes (e.g: IE11) The minimum ECMAScript version needed to run Chevrotain is
now ES2015 (ES6). This should not affect anyone running on a modern engine,
meaning modern NodeJS versions or popular evergreen browsers.
Various TypeScript signatures are now more accurate and strict which could potentially cause compilation errors with
some grammars implemented in TypeScript, e.g:
OPTION methods now return OUT | undefined instead of just OUT.
A RULE implementation function in CstParser is now defined as returning void.
The ARGS for SubruleMethodOpts options type is now better enforced via generics in the subrule definition.

https://chevrotain.io/docs/changes/BREAKING_CHANGES.html
I only had to fix some optional types, build and tests are passing green.

@maxence-lefebvre - Thanks for the PR! everything looks good. I will review one more time and run tests locally and if all goes well will publish a new version this afternoon.

Thank you ! Take your time, I am in CEST timezone so the day is almost over here


Add support for capturing the output of Processes

This code supports capturing stderr and stdout logs of Processes.
When Process object is created, Start Information is configured with event handlers to subscribe to events and redirect streams of standard output/errors to the event handlers. Whenever events are raised to event handlers, it writes them to the PF_ConsoleLog.txt until process is terminated or error is captured.
Sample Output For Windows Process:
PF_ConsoleLogs.txt
Sample Output For Linux Process:
PF_ConsoleLogs(Linux).txt

Do you have an example text output for a run?

@yegarci  updated the PR description - check out the link!

Sorry, I mean do you have an example output where the new stderror and stdout streams are written?


remove pr_nr step as it is not needed

I think, that this step is needed for checkout the correct commit in the git repo, which must be done in action, which uses testing-farm-as-github-action, and not in testing-farm-as-github-action itself.

@fernflower What do you mean by this change? Does it make sense to you to get the pull request number directly in this GitHub Action? WDYT? From my point of view, only checkout will be needed in our upstream projects. And the pulling references will be done in GitHub Action. Or it can be configurable.

So sorry for late response.
I believe we can get rid of this step, we (leapp team) had pr_num in the pre-tft version of our workflows to pass as an env var to the env setup playbooks (that was part of package installation workaround), but we don't need that any longer as testing farm handles atifact installation for us.
So +1 from me.


Image link to googleusercontent.com not downloaded

Hello
There is a link on telegra.ph with images linked to lh3.googleusercontent.com google storage and your script went in error when try to download image. But if i copy/paste the link into browser it show the image perfectly.
Here you are one of that links not downloaded:
https://lh5.googleusercontent.com/6ZVrJWQlQ5JieOpMfPtGNPER0o17NOyyNfwth7U9rge6GT3zdo6iWzzbqw0eqISAfPF6Cyj093Pz_Y_qnTogtRueHmHG85H_YAlJxz_pM-Ifnoh4JZuIJ5xxJzqJbh7Vk0RrhntD
Thanks

I found a workaround and it will work in a new version. It will come out right when issue #2 is closed.
Waiting for your answer there :)


ESLint v8.53.0 deprecated stylistic rules

Too much added complexity and maintenance work in core - for something that's better handled by dedicated formatters - was the stated reason (see: eslint/eslint#17522), with the TypeScript and React plugins following suit.
There are two alternatives for the future, then:

@stylistic/eslint-plugin as a 1:1 replacement - an immediate solution
Create a new plugin for a tool like Prettier - which has the advantage of handling Svelte as well

Always good to note. Let me know if you see any actual deprecation warnings. I don't use ESLint for code formatting per se, but there may be some long standing rules that I need to redefine / change plugins if necessary.
The TyphonJS ESLint config setup is what I use for development and do have setup in the Foundry TRL demo repos, but it's not a requirement.
Once the Svelte 5 transition occurs I'll also be evaluating adding Svelte support and more, but waiting on all of that, so quite likely unless there are deprecation warnings the TJS config files will be updated later this year.


Proposal: groupBy query adjustments, introduce merging buffers

Currently groupBy works like this:

factory.createRunner (run on historicals) uses GroupByQueryEngine to return a Sequence<Row> for each segment. For each row returned by the cursor, GBQE uses an array of the dim ids as a key into an on-heap positions map, where the values are positions of aggregators in an off-heap computation buffer. The contents of this buffer are emitted out to the Sequence either when the buffer fills up, or when the positions map size reaches maxIntermediateRows. (I guess this is here to prevent the positions map from taking up too much heap space)

factory.mergeRunners (run on historicals) submits factory.createRunner-based tasks to the processing thread pool that use an accumulator to add rows from their sequences to a single IncrementalIndex, allocated by mergeRunners. If at any point there are more than maxResults rows in this index, the query fails. This index might be onheap (default) or might be varying degrees of offheap (#2325, #2847). After the processing tasks are all done, the outer runner emits rows from that index. #2571 changed this index to be unsorted rather than sorted, because nothing actually depended on the fact that it was sorted.

toolchest.mergeResults on the historical used to do an unnecessary merge again in a second index, but #2962 adjusted that so it is a no-op.

toolchest.mergeResults on the broker handles merging by adding all incoming rows to an IncrementalIndex. If at any point there are more than maxResults rows in this index, the query fails. It then applies limits by possibly re-sorting and limiting the rows coming out of that index. If the limit involves a re-sort then a copy of the results get materialized, otherwise the limit is applied to the stream. If there is a nested query, that also always materializes the inner query result set.

There are some things about this that can be improved:

The maxResults limit is driven by memory constraints right now, it'd be better to allow temporary storage of results on disk so we can return larger result sets.
The number of merging indexes allocated by query.mergeRunners and toolchest.mergeResults is limited only by the jetty thread pool size, which can be large and can cause us to allocate too much memory for merging.
Merging on-heap makes it difficult to manage memory, but previous attempts at merging off-heap have had performance issues. #2847 looks promising for unsorted results but is still behind the on-heap implementation for sorted results.
The broker could avoid materialization when we are ordering on dimensions rather than metrics, by pushing sort down to historicals and then doing a stream merge instead of a materialized merge.
The broker could push down limits too when we are ordering on dimensions.
The broker could avoid materialization for nested queries and instead apply the outer query directly to the incoming stream (when the inner query can be streamed).

I've been working on an implementation that tries to address 1, 2, 3, and 4 (not 5 or 6 right now). So far in benchmarks performance is similar to or better than the existing on-heap version. In the proposed new implementation:

factory.createRunner does something pretty similar, except it stores its equivalent of positions off-heap too and as such, doesn't have a maxIntermediateRows config.

DruidProcessingModule provides a new @Merged BlockingPool<ByteBuffer> that can be used for merging. It has a fixed maximum size and it's blocking, so jetty threads can request one when they want to do a query that will require a merge buffer, and then wait in line if one is not available (without submitting tasks to the processing thread pool). Queries that don't need merging buffers don't have to wait.

factory.mergeRunners is similar, except instead of using IncrementalIndex for merging, it uses an off-heap hashtable that is not an IncrementalIndex. It uses buffers from the merging pool and it returns sorted results. In this implementation the dictionary is still on-heap but everything else is off-heap. The merger spills to sorted files on disk when either the merge buffer is full, or the dictionary size exceeds a maxMergingDictionarySize limit. The merger can take advantage of features of the groupBy query workload that do not apply to regular indexing: the number of dimensions is known in advance, it's not necessary to be able to read anything until all writes have been done, and it's not necessary to be able to read from more than one thread.

toolchest.mergeRunners uses a ResultMergeQueryRunner to merge result streams, which it assumes are ordered on time and dimensions. I think this OK if getResultOrdering in the query is overridden, since the broker uses MergeIterable with that comparator when stitching together streams from data nodes and cache, so they should get ordered properly before they make it to the toolChest. The broker still materializes and sorts results if the limitSpec involves an order by on something other than the dimensions. Otherwise it will just limit the stream.

:+1: sounds good, I would imagine still having a limit maxResultSizeBytes used at historicals while merging results from runners to prevent somebody from overloading historical with a very large query.

yeah, I think that'd be useful, or even a config to disable temporary disk storage. In that case the processing buffer size acts as a limit on how much we can group.

((maybe too deep of a question at this level)) Does this mean the query results to be merged can be put into the off-heap buffer without doing deserialization from historicals?

@drcrallen I don't understand the question, put into the off-heap buffer by what?

Thinking about it more I think I was asking the wrong kind of question. I was getting at a question about if results are going to be serialized multiple times if they are all stored off-heap, but I think I mis-understood what part of the results go into the ByteBuffer blocking pool.

Other question: Does this just push the "do I have enough memory?" question from the heap size to the individual BlockingPool<ByteBuffer> buffer size?

Yes, pretty much, although the behavior when the answer is "no" is better – the query will not start until it can allocate the memory it needs, instead of throwing OOME somewhere in the middle.
The idea is really just that the @Global StupidPool<ByteBuffer> is used by processing threads and bad things happen if it is used by anything that is not a processing thread, or if it is used more than once by a processing thread (allocations without bound, deep in the query runner stack).
The merger needs to allocate a buffer that is usable by processing threads, but is not associated with any one of them in particular. If it drew from the @Global StupidPool<ByteBuffer> then we could potentially draw as many of those buffers as we have jetty threads, which is a whole lot of buffers. IMO it's better to use a limited-size blocking pool such that queries that need merge buffers need to wait until one is available before they start running.
If these are the same size as the processing buffers then instead of the max number of buffers being druid.processing.numThreads then it would be something like druid.processing.numThreads + druid.processing.numMergeBuffers.

@gianm How are you wanting to handle the blocking merge pool's ByteBuffers? Are the individual pool elements dynamically expandable?

I was thinking they would be fixed size just like the processing buffers (probably the same size to keep things simple). If a merge buffer isn't big enough to do the merging work, the query could either fail or it could write to temp files on disk (query's choice). If however much disk space we allow the queries to use is ALSO not enough then the query fails.

closed by #2998


firefox problem

With Chrome all works fine, but in Firefox i have error when i broadcast a comment:
Error: 3030000 tx_missing_posting_auth: missing required posting authority
Missing Posting Authority sci
{"id":"sci","posting":{"weight_threshold":1,"account_auths":[["streemian",1]],"key_auths":[["STM8MfcHZyYioPBsmoYp8AZf4mbwZdsarunFRvuWUySqb3nwPeRwb",1]]},"active":{"weight_threshold":1,"account_auths":[],"key_auths":[["STM82cz4GKWWiEoikHfehMJ9PapecmoUsf4bE7NDoZD47PTBqSTK4",1]]},"owner":{"weight_threshold":1,"account_auths":[],"key_auths":[["STM67rQk47AyFkSzw61CMdpKJEtFwwJr3QyFUpt9Wb54J9LDEyeea",1]]}}
th_a  transaction.cpp:129 verify_authority
{"ops":[["comment",{"parent_author":"","parent_permlink":"test","author":"sci","permlink":"test-article-number-y","title":"test article (number y)","body":"tsst","json_metadata":"{"tags":["test","news","technologies"],"model":"news","app":"mapala","}"}]],"sigs":["STM7pQESd1aqnYbf9DFmZFScNpSudZn4PzCYgJ3udYos239913WYT"]}
th_a  transaction.cpp:172 verify_authority
And key is always different. I had a same trouble with GolosJs, while im not install correct Chain-Id. Firstly, i think this is my local problem, but with SteemJs and transaction to Steem network - i have same answer in FireFox..
I use it PHP Yii2 application, just include steem.min.js and command:
            steem.broadcast.comment(wif.plaintext,
            trx['parentAuthor'],
            trx['parentPermlink'],
            trx['author'],
            trx['permlink'],
            trx['title'],
            trx['body'],
            trx['metadata'],
            function(err, result) {
                console.log(err, result);

                if (err == null){
                    setTimeout(redirect, 6000);
                } else alert(err);
        });

Simple, and work with Chrome.. But why firefox does not work?

+1 FireFox issue with escrow_transfer transaction.

@adcpm  Hello. I see that issue in parsing date from string. Firefox think that date was passed on local time. And I see that you had commented fix for that issue (I don't know why you comment it?)
src/auth/serializer/src/types.js:408
if(typeof object === "string" && !/Z$/.test(object))
object = object + "Z"
Could you please tell why did you (or somebody other) do this? Thanks.
@MrXtar @dacom-dark-sun if you want use current version - Just add 'Z' symbol to you time strings (I mean - ratificationDeadline, escrowExpiration).

Chrome assumes a date string is a Zulu date with or without the Z but FF requires the Z.
I don't know why that is commenting out, I'm trying to trace it back into the Bitshares repository where we may have a commit message.
It is likely that we could enable it since this kind of issue would cause the date to serialize incorrectly in FF ever time.  In other words, I think you are the first to depend on this so it should be safe to enable it.  Lets see what we can learn from this commit history first though.

BTW.. If anybody wants to use current version - just pass Date objects to steem-js functions, not strings.