file:// URLs

Are there plans to support local filesystem URLs?

+1
Really surprised to discover this doesn't work.

There might be some security implications.

It's not on my roadmap at the moment, node-fetch sits on server-side and local url usage is limited.

This should work as it works in the browser.

I guess we've decided that file:// will not be supported.

Just to clarify, if this works consistently across major browsers we will re-open.
But more as a clear warning that we don't support file:// protocol, just like we don't support relative url.

@bitinn @TimothyGu I don't I understand the aversion to file:/// urls.
If we're on the server side (using Node.js + Express), and I have to call another rest endpoint whose swagger is not easily obtainable, why can't I take care to put it under source control and simply use it instead of the undownloadable spec from the service I'm calling?
This limitation seems arbitrary.  Please clarify its justification.

@matthew-andrews That sounds like a problem with how node-fetch is used, not with node-fetch. FYI Node.js v8 will be released with support for file URLs in fs module methods.

The Response constructor can take a Readable stream so why not simply do something like this?
let {Response} = fetch
let stream = fs.createReadStream('myfile.txt')
let promise = new Response(stream).text()

I don't think file protocol should be supported, sorry
I for once use node-fetch as a proxy, user can request any url they want with my api
wouldn't want them to get access to the filesystem...

I don't see why it can't be supported behind an option to enable it.

_NB: I'm @matthewadams, not @matthew-andrews. Sorry for the noise, @matthew-andrews  _
@TimothyGu I'm confused by your response.  Please see https://github.com/swagger-api/swagger-js/issues/1044 for justification.  Do you have a pointer for Node.js v8 supporting file:// URLs in fs?

@jimmywarting I still don't understand why node-fetch shouldn't support file:// URLs.  You could easily prevent your users from using file:// URLs simply by testing for it in your API.

You could easily prevent your users from using

As you could easily switch to using fs by testing the URL in your api...
IMO I think security comes first. The file protocol isn't part of the spec

@jimmywarting the spec is already broken here: https://github.com/bitinn/node-fetch/blob/master/LIMITS.md

+1 again ... it still seems a no-brainer, to me. URL stands for Uniform Resource Locator, after all. Tim Berners Lee would be rolling over in his grave. (If he were dead, that is.)

Much thought have gone into this issue, for sure. I'd like to condense the most important reasons I believe URLs of file scheme should not be implemented in node-fetch. I would of course be thankful if @bitinn or @zkat would like to weigh in and offer their opinions here as well.
1. Accessing the local filesystem eases security breaches.
Take @jimmywarting's (admittedly anecdotal and contrived) use case:

I for once use node-fetch as a proxy, user can request any url they want with my api
wouldn't want them to get access to the filesystem...

Really, I cannot believe many people are using node-fetch as a proxy (I might be wrong of course, but IMO there are many much better ways to create a proxy in Node.js). But I find it easy to imagine people who are using a URL from an untrusted source (the network for example) and just plug it into fetch(). In many cases, that practice is fine. It would not be fine if file:///etc/passwd is accessed.
@stevenvachon proposed using an option to allow file URL scheme. That may solve this issue, but see my response below.
2. There is no recognized standards for interpretation of file-scheme URLs.
As outlined in https://github.com/sindresorhus/got/issues/227 filed by @stevenvachon, WHATWG's Fetch Standard offers no discussion of how file URL schemes should be interpreted on different platforms. In fact, in the PR adding documentation for Node.js fs module file URL support, I explicitly recommended documenting how URLs are interpreted on different platforms, precisely because of the competing interpretations.
Now that file URL scheme is supported officially in Node.js, I foresee a de-facto standard being established at least in the Node.js community. But then in less than a month when Node.js 8.0.0 is released, users can use fs.readFile(new URL(fileURL), (err, buf) => { if (err) ... } ) (or any promisified variant), which I consider to be an improvement over fetch(fileURL, { allowFileScheme: true }).then(res => res.buffer()).then(buf => { ... }).catch(err => { ... });.
3. There is no precedent for support of file-scheme URLs in Node.js community.
None of the most popular URL-to-response libraries I can think of in under a minute do not support file-scheme URLs:

request with 24M monthly downloads (https://github.com/request/request/issues/2229)
got with 5M monthly downloads (https://github.com/sindresorhus/got/issues/227)
superagent with 3M monthly downloads
then-request with 0.3M monthly downloads

4. Browser fetch() does not support file-scheme URLs.
There are a lot of people who use node-fetch as a server-side polyfill to allow for isomorphic requests. It would be a surprising behavior if node-fetch has the capability to access the file system browsers' fetch() cannot. However, neither Chrome nor Firefox supports fetch() with file-scheme URLs. GitHub's fetch polyfill refuses to do so, also: https://github.com/github/fetch/pull/92#issuecomment-140665932

@matthewadams

NB: I'm @matthewadams, not @matthew-andrews. Sorry for the noise, @matthew-andrews

Oops, apologies. Shouldn't have depended on the autocompletion.

I'm confused by your response. Please see swagger-api/swagger-js#1044 for justification.

What I meant to express is that Swagger should be aware of node-fetch's limitations, and decide if it wants to allow file URLs. I do not see any indication of such a decision made in the linked issue by the maintainers of Swagger, and if I were to design the Swagger API, I would allow both HTTP(S) URLs and a plain file path instead of resorting to file-scheme URLs.

Do you have a pointer for Node.js v8 supporting file:// URLs in fs?

Code was added in https://github.com/nodejs/node/pull/10739. Docs were added in https://github.com/nodejs/node/pull/12670.

@stevenvachon

I don't see why it can't be supported behind an option to enable it.

Adding an option that defaults to true would not change issue 1 above.
If it defaults to false, issues 1 and 4 would indeed be resolved. However I would prefer not to add to the list of non-spec options we have unless absolutely necessary. With the exception of size, the options we have all aim to reimplement browser defaults that users on Node.js may or may not want.

the spec is already broken here: https://github.com/bitinn/node-fetch/blob/master/LIMITS.md

This issue is not an issue of spec-compliance. Even if it is, as the steward of node-fetch, I would exercise my best decision when it comes to following the spec, which according to the reasons I outlined above leads me to the side of "against."

@Klortho

URL stands for Uniform Resource Locator, after all. Tim Berners Lee would be rolling over in his grave. (If he were dead, that is.)

Yes, URL indeed stands for that. But it should come as no surprise that certain applications only support certain URL schemes. And speaking of Tim Berners-Lee, I would like to add a shameless plug to a particular tweet of mine. :smile:

I still pretty against it. I don't see why we want to wrap around fs module (we wrap around http module and that's enough of a maintenance headache given the changing Fetch Spec).
Plus the issue 2 in Timothy's reply: Fetch Spec gave us nothing to work with.

How to enable file URLs - https://gist.github.com/joshua-gould/58e1b114a67127273eef239ec0af8989

@bitinn Could you please reconsider this and unix socket support? Interacting with things like the REST API on the Docker daemon socket are impossible without it.

@masaeedu Still a nope on File URL.
But if you really need UNIX socket support, try got, they are modelled after Fetch. https://github.com/sindresorhus/got#unix-domain-sockets

3 years later still no way forward, shame

@TimothyGu replying to your comment https://github.com/bitinn/node-fetch/issues/75#issuecomment-299755555

However, neither Chrome nor Firefox supports fetch() with file-scheme URLs.

That's news to me. Yes, Chrome disallows fetch-ing any file:// scheme based url, but Firefox definitely allows fetch('file://') as long as their same origin policy is followed.
As your comment is from 2017 and my reply from 2019 there is the potential that something has changed since. But I definitely have used fetch on file:// in 2017 so I guess nothing much has.

so if I want to fetch a ftp:// file, how should I go around doing this?

so if I want to fetch a ftp:// file, how should I go around doing this?

Every once in a while I have the misfortune of using node-fetch directly or indirectly through another package. What happens every single time is spending 2hrs+ only to find out file:// scheme is not supported in a package named "NODE-fetch". Now read that again. It says Node in the name. Which is supposed to run in Node, right? Because the name says so! It does not say web-fetch. It does not say browser-fetch. It says NODE-fetch.
Where is Node ran? server side. right? What do you have access to in the server side always, a filesystem, right?? right???
SO, why exactly does this not support local filesystems? Why does this have to work like a browser? Who cares if this works like a browser?? If I wanted browser behavior, I would not have pulled a package called NODE-fetch. I would have pulled something called browser-fetch or something else.
And what security implications exactly does this lack of support refer to?  If you can't point exactly what security implication you're trying to address, you are addressing absolutely nothing, just a waste of everyone's time.
This is not even the only GH issue on this for this package. Clearly lots of people are suffering from this problem. On another thread a maintainer cited the reason for this due to "servers not having an origin URL". Excuse me, but then what is process.cwd() in the context of a local filesystem for a package THAT IS SUPPOSED TO RUN SERVER SIDE ALWAYS as its name implies?

I understand that you're frustrated @3p3r and have had a difficult experience with node-fetch and just want to vent a little bit. However, it's important to keep a respectful tone when discussing issues with others on open source projects.
Regarding your question about why node-fetch have decided not to support the file:// scheme, there are several reasons for this. First and foremost, allowing fetch requests to access local files can introduce security vulnerabilities. This is because it opens up the possibility of a malicious user accessing files on your computer that they shouldn't have access to.
such as in the example of building CORS proxies \w the fetch api.
While I understand that you would like node-fetch to support the file:// scheme, it's important to consider the potential risks and drawbacks of doing so also.

you can get around this with a simple wrapper around fetch that first checks the protocol and if it starts with file:// then you could create a blob backed up by the disc and construct a new Response with the blob. Using blob instead of a stream will give you the added content-length and content-type header as a bonus for using blob (thanks to blob.size and blob.type)
import fetch, { Response, blobFrom } from 'node-fetch'
import fs from 'node:fs'
import process from 'node:process'

const base = process.cwd()

export default async function fetchWrapper(url, options) {
  const parsedUrl = new URL(url, base)
  if (parsedUrl.protocol === 'file:') {
    // If the URL starts with 'file://', read the file from the local file system
    const filePath = decodeURIComponent(parsedUrl.pathname)

    if (!fs.existsSync(filePath)) {
      return new Response(null, { status: 404, statusText: 'NOT FOUND' })
    }

    const blob = await blobFrom(filePath, { type: 'application/octet-stream' })
    return new Response(blob, { status: 200, statusText: 'OK', url })
  } else {
    // For all other requests, use node-fetch as usual
    return fetch(url, options)
  }
}

I am absolutely not trying to bash the maintainers. I am so insignificant in the pool of 36M+ people that download this package that whatever I say makes no dent in anything.
I am trying to point out a design flaw here. In a server environment where this package is used 99% of the time (and should be 100% of the time!), accessing resources over a network is most definitely less secure than accessing the local filesystem. You cannot argue that fetching network resources in Node is safer than accessing local filesystem.
This behavior only makes sense in browser, because there are further safeguards for networks access. The browser platform fundamentally does not allow file access and its networking stack is strict due to all layers of standards that went into it all these years.
But those networking safeguards do not exist in Node, so arguing that accessing network is safer in Node vs accessing local filesystem is moot. A CORS proxy built with node-fetch, hosted somewhere online should not have sensitive files next to it. Or it should not have filesystem permission setup in a way that you can fetch /etc/shadows. That's not on this package to solve. That's on users to solve. Let the user handle edge cases. Inversion of Control please.
That won't make this package any less secure. If I could use node-fetch to cause stack overflow to execute arbitrary shell commands, now that would make this package insecure.

Chrome (Chromium-based browsers) extensions support fetch() with file: protocol passed Issue 1227761: Fetch() should support file scheme for extensions
manifest.json
{
  "name": "Service Worker-based background script",
  "description": "Test that fetching a file scheme URL succeeds",
  "version": "1",
  "manifest_version": 3,
  "host_permissions": ["file:///*"],
  "background": {"service_worker": "service_worker_background.js"}
}

Keep in mind Chromium-based browser supports fetch() for chrome-extension:, too. Chromium might arbitrarily create any protocol, e.g., quic-transport:, then remove the protocol and create another protocol, e.g., isolated-app:. There is also ipfs: scheme support https://chromestatus.com/feature/5105580464668672.

Looks like node v22.0.0-nightly202311151d8483e713 executable implementation of fetch() supports file: protocol
>  var scriptText = await (await fetch("file:///home/user/exports.js")).text();
undefined
> scriptText
"const fn = () => 123;\nexport {fn};"
> const mod = await import(URL.createObjectURL(new Blob([scriptText], {type:"text/javascript"})));
undefined
> mod
[Module: null prototype] { fn: [Function: fn] }
> mod.fn()
123

Looks like node v22.0.0-nightly202311151d8483e713 executable implementation of fetch() supports file: protocol
>  var scriptText = await (await fetch("file:///home/user/exports.js")).text();
undefined
> scriptText
"const fn = () => 123;\nexport {fn};"
> const mod = await import(URL.createObjectURL(new Blob([scriptText], {type:"text/javascript"})));
undefined
> mod
[Module: null prototype] { fn: [Function: fn] }
> mod.fn()
123

based time to dump this library

@jimmywarting With all due respect I don't think the comment at https://github.com/node-fetch/node-fetch/issues/75#issuecomment-1818512006 should have been edited by you.

I don't appreciate such a bad tone. it's hurtful and not fun at all to anybody in a open source community where you put down a lot of time and energy into something for free.
how would you feel if i posted something like it in any of your own repository?

I don't think the "tone" is "bad". The point of node-fetch is a polyfill for Node.js lack of standardized WHATWG Fetch Standard support, correct?
Node.js Fetch implementation now supports fetch() natively, including full-duplex streaming, and fetching file: protocol.
I would not mind at all if developers dump any code I have written and published due to native browser or non-browser JavaScript runtime implementation happening.

To me the word "dump" have a negative Interpretation. specially in this context.
As you may already know file: are left as an exercise for the reader (in the spec)
Some of all the original members and authors have all agreed to not support file: cuz of security concerns.
it's unclear what the response should be if it find the file, what header it should have and what should it return when it's not found.
it's very bad if a fetch call makes a request to a server and the server responds with a redirect header going from something like https: to file:

file: is a secure context per the controlling W3C specification, or rather is no specified as insecure.
It makes sense that it is more "secure" to fetch local files than to expose the entire system to remote networking requests.

it's unclear what the response should be if it find the file, what header it should have and what should it return when it's not found.

Handle the request like any other protocol. 404 if not found.
The developer should be in control over the API surface. The user determines what is "secure" and what is "insecure", not some external specification controlled by a handful of individuals who are not in control of the users' machine.

it's very bad if a fetch call makes a request to a server and the server responds with a redirect header going from something like https: to file:

That's possible anyway. In the browser using an extension. Outside of the browser using a proxy.
Again, the user makes that decision on their own machine.
Full-duplex streaming is a little more involved specification-wise, though shouldn't/doesn't have to be. Folks have implemented full-duplex streaming to the browser. Specification language is W.I.P. by certain parties. Both node and deno support full-duplex streaming. bun doesn't.
My point is I'm not a proponent of censorship. Everybody thinks their own ideas are great.

Goebbels was in favor of free speech for views he liked. So was Stalin. If you’re really in favor of free speech, then you’re in favor of freedom of speech for precisely the views you despise. Otherwise, you’re not in favor of free speech.
-Noam Chomsky

It makes sense that it is more "secure" to fetch local files than to expose the entire system to remote networking requests.

Makes no sense to me. cuz why would you exposing a file system?
if you can do fetch('file://') then you have as much read access as having access to any file system.
sure i may be more secure in a way that you can not list, delete or edit files but still...

Handle the request like any other protocol. 404 if not found.

why not throw? and what about the response? should it contain content-type, content-length, last modified date, accept byte range? how much should it expose?

That's possible anyway. In the browser using an extension. Outside of the browser using a proxy.

I know that deno supports file, and it have protections agains redirect attacks like https:// -> file://
if we can have that then all kudos to that!
and if we can implement something like a permission control to allow fetch('file://...') by passing some explicit option to enable it or having a --flag then sure by all means we could reconsider it. it should not be enabled by default.
TimothyGu have already gone trough why supporting file:// would be bad
Why are we talking about duplex streams? it's have nothing to do with file:

I haven't tested node v22.0.0-nightly202311151d8483e713 cuz it's not released?
but i tried latest undici which is what nodejs are using... and fetch did not have any support for file://
So i have my doubts if it's working

Makes no sense to me. cuz why would you exposing a file system?
if you can do fetch('file://') then you have as much read access as having access to any file system.

Then you can not fetch file: on your own machine.
Extending your own personal preferences to an entire API is selfish and to me denotes you want to control what users do on their own machine if they use your library.
Why?
You are not responsible for what users' fetch on their own machines.
Relevant specification issue https://github.com/w3c/webappsec-secure-contexts/issues/66.

why not throw? and what about the response? should it contain content-type, content-length, last modified date, accept byte range? how much should it expose?

You can throw or not throw. Whatever. The point is arbitrarily barring fetching file: protocol is something you can do on your own machine. Yes, you can include ordinary headers you would for any other request.

I haven't tested node v22.0.0-nightly202311151d8483e713 cuz it's not released?
but i tried latest undici which is what nodejs are using... and fetch did not have any support for file://

It is released. Fetch Node.js nightly archive and test for yourself to resolve any doubts you might have.

Why are we talking about duplex streams? it's have nothing to do with file:

I'm just saying the reason for node-fetch has passed. We can full-duplex stream and fetch file: using the lastest node executable standalone, without any libraries.

And you can say whatever you like, but plz do it elsewhere so that the person dose not get offended by it.
i don't want to keep doing open source stuff if ppl is going to have a "bad" atitude

I think maintainers need to learn how to handle constructive feedback.
You have done great work in the open source software domain.
Editing honest user feedback is problematic on many levels. IMO you would have done better to just mark the comment as off-topic rather than editing the users' comment because you felt "offended". I don't think the author of the comment was trying to throw shade. node-fetch probably did help a lot of developers wanting to use fetch() in Node.js environment. Now Fetch Standard is implemented. Why use node-fetch? Because of legacy dependencies?
Anyway, I ain't trying to give you grief. I was startled by you editing a users' comments here and conveyed my opinion on that act.
Have a great day, cheers.

I mean, yeah, at least, disable it by default, but write options to enable it if you want, doesn't node has permissions now? if you want to be safe you can use those permissions.

I mean, yeah, at least, disable it by default, but write options to enable it if you want, doesn't node has permissions now?

If you are using node the question must be asked, why are you using node-fetch? node implements fetch(), marked as "stable" now https://github.com/nodejs/node/commit/740ca5423aabc06af48f6dacfb17335bbaa25c0c even though there is some discussion still ongoing based on lack of input, allegedly https://github.com/nodejs/node/pull/49867.
I didn't use any special flags when I tested. I fetch the Node.js nightly archive every day or so though, so the example I posted is using the node nightly release.
Isn't the whole point of polyfills that native support is not shipped?

Node-fetch was created long before when fetch was only a browser thing and was very new
Saying that is's a pollyfill for what nodejs have built in today is "wrong". It dose not rip out what exist in node today and and put it on npm or anything like that. It dose not try to adhere to nodejs implementation. We are just following the fetch specification.
we created a server side implementation first and our goal where to give browser fetch to the server. And prefixed it with node cuz it was specifically built for node:http in mind

I see. So node-fetch and fetch() implementation in node executable are not intended to be, and are comparable. They do different things for different contexts and use cases, are not mutually exclusive.

Looks like fetching file: protocol isn't working anymore in node v22.0.0-nightly202311250bb5d88871.

For anybody interested in making this happen for Undici fetch() here are the relevant lines that throw when passing a file: URL https://gist.github.com/guest271314/2779cfbb4a4f8ba3496e253e546c0104.

For anybody interested in making this happen for Undici fetch() here are the relevant lines that throw when passing a file: URL https://gist.github.com/guest271314/2779cfbb4a4f8ba3496e253e546c0104.

at new URL (node:internal/url:804:36)
hmm maybe somthing that can be patched?

@Kreijstal A few ways to do this for Node.js' Undici fetch() implementation https://gist.github.com/guest271314/a4f005d9a6b5b433ae6d6e6c5c6d7595.
At https://github.com/nodejs/undici/blob/main/lib/web/fetch/index.js#L129 add the following to short-circuit for file: protocol
  const url = new URL(input);
  if (url.protocol === "file:") {
    return import("node:fs").then((fs) => {
      p.resolve(new Response(fs.readFileSync(url)));
      return p.promise;
    });
  }

Using curl
import { spawn } from "node:child_process";
import { Duplex } from "node:stream";

async function fetchFile(path) {
  // https://github.com/chcunningham/atomics-post-message/blob/main/server.js
  const mimeTypes = {
    "html": "text/html",
    "jpeg": "image/jpeg",
    "jpg": "image/jpeg",
    "png": "image/png",
    "js": "text/javascript",
    "wasm": "application/wasm",
    "css": "text/css",
  };
  try {
    const { stdout, stderr } = spawn("./bin/curl", ["-NSs", path]);
    if (stderr) {
      // Handle file not found: "curl: (37) Couldn't open file /home/user/bin/nm_hosts.js\n"
      const err = await new Response(Duplex.toWeb(stderr).readable).text();
      if (err) {
        throw err;
      }
    }
    return new Response(Duplex.toWeb(stdout).readable, {
      headers: {
        "Content-Type": mimeTypes[path.split(".").pop()] || "text/plain",
      },
    });
  } catch (e) {
    throw e;
  }
}

export { fetchFile };

dd command
  const url = new URL(path);
  try {
     const { stdout, stderr } = spawn("dd", [`if=${url.pathname}`, `count=${1024**2}`, "status=none"]);

GNU Coreutils head
  const url = new URL(path);
  try {
    const { stdout, stderr } = spawn("head", ["-c", 1024**2, url.pathname]);


python API

Bug Description
I encounter errors when calling flow through Python API
{
"detail": "Invalid or missing API key"
}
What does this API key refer to? Is it the Langflow API
Reproduction
I encounter errors when calling flow through Python API
{
"detail": "Invalid or missing API key"
}
What does this API key refer to? Is it the Langflow API
Expected behavior
I encounter errors when calling flow through Python API
{
"detail": "Invalid or missing API key"
}
What does this API key refer to? Is it the Langflow API
Who can help?
No response
Operating System
windows 10
Langflow Version
1.0.9
Python Version
None
Screenshot
No response
Flow File
No response

same problem reported in #3143 and #3172


debug: access symbols for libarrow_python_flight.dylib

Describe the usage question you have. Please include as many useful details as  possible.
Hi,
I have been trying to get access to the symbols of this dylib for a while now and failed miserably. I have followed the guide for developers at this link https://arrow.apache.org/docs/dev/developers/python.html and I have successfully built and generated a pyarrow package locally that I can use in my pyenv. I was successfully able to print symbols and debug the source code files for some of the other dylib (e.g. libarrow_flight.1400.dylib) but when I get to libarrow_python_flight.dylib which is where I am getting a segmentation fault that I am trying to debug, I just see assembly.

And with the following env variables when building pyarrow:
export PYARROW_WITH_PARQUET=1
export PYARROW_WITH_DATASET=1
export PYARROW_PARALLEL=4
export PYARROW_WITH_ARROW_FLIGHT=1
export PYARROW_WITH_FLIGHT=1
export PYARROW_BUNDLE_CYTHON_CPP=1
export PYARROW_BUILD_FLIGHT=1
export PYARROW_BUNDLE_ARROW_CPP=1

As visible from the screenshot I am able to see the source code and access the variables in that frame for some dylib, but not for the one I want:

Only assembly seems to be available.
I am working on a:

Mac M1 with Ventura 13.6.
Python 3.11

I appreciate any help you could give.
Thanks
Component(s)
C++, FlightRPC, Python

Could you rebuild your PyArrow with export PYARROW_BUILD_TYPE=debug?

Hi @kou, thanks for the quick reply. Wouldn't this be achieved by this step as showed in the documentation via the --build-type parameter, rather than using the environment variable?
python setup.py build_ext --build-type=$ARROW_BUILD_TYPE \
         --bundle-arrow-cpp bdist_wheel

If the answer is yes, then that is how I have been building pyarrow as well, if not I will try in a couple of hours and report back.

--build-type should work too.

--build-type should work too.

I see, that is what I have been using, any other ideas?

How about trying PYARROW_BUILD_TYPE?

Okay I have tried rebuilding with the export PYARROW_BUILD_TYPE=debug but it didn't make any difference unfortunately. Still see only assembly. Is there a command I can run on the dylib to ensure it has been built correctly in debug mode?

Could you show the command line you tried as-is and full log of the command line?

Yep, okay so posting here only the pyarrow building part, let me know if you need to see the arrow one as well:
(pyarrow-dev) XDWRL412MF:Projects dbarone$ pushd arrow/python
~/Projects/arrow/python ~/Projects ~/Projects ~/Projects
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_WITH_PARQUET=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_WITH_DATASET=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_PARALLEL=4
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_WITH_ARROW_FLIGHT=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_WITH_FLIGHT=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_BUNDLE_CYTHON_CPP=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_BUILD_FLIGHT=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_BUNDLE_ARROW_CPP=1
(pyarrow-dev) XDWRL412MF:python dbarone$ export PYARROW_BUILD_TYPE=debug
(pyarrow-dev) XDWRL412MF:python dbarone$ python setup.py build_ext --build-type=debug --bundle-arrow-cpp bdist_wheel &> output.log

This is the output.log file generated from the python command above.
output.log

-- Build Type: DEBUG

Your build used debug build.
It seems that LLDB can't find source files for libarrow_python_flight.dylib. You may be able to use settings set target.source-map for it: https://lldb.llvm.org/use/map.html#remap-source-file-pathnames-for-the-debug-session

To be honest I don't fully understand what is the supposed build directory, looking at the output.log file I do see this one mentioned:
Build output directory: /Users/dbarone/Projects/arrow/python/build/temp.macosx-13-arm64-cpython-311/debug

but I do see the actual source files in /Users/dbarone/Projects/arrow/python/build/lib.macosx-13-arm64-cpython-311/pyarrow/src/arrow/python/, I tried to set those using the following command:
settings set target.source-map /Users/dbarone/Projects/arrow/python/build/temp.macosx-13-arm64-cpython-311/debug /Users/dbarone/Projects/arrow/python/build/lib.macosx-13-arm64-cpython-311/pyarrow/src/arrow/python/

but it didn't make any difference.

Could you try cd python && rm -rf build && PYARROW_BUILD_TYPE=debug PYARROW_...=... pip install . instead of python setup.py build_ext ...?
It works for me.

I found this issue while trying to figure out how to debug C++ code inside PyArrow and I think I'm seeing something similar to the above. With a debug Arrow C++ and PyArrow build, I can hit a breakpoint but I don't see source code mapping:
(venv) bryce@debian ~/s/a/a/python (main)> lldb -- $(which python)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ModuleNotFoundError: No module named 'lldb'
(lldb) target create "/home/bryce/src/apache/arrow/python/venv/bin/python"
Current executable set to '/home/bryce/src/apache/arrow/python/venv/bin/python' (x86_64).
(lldb) b ConvertPySequence
Breakpoint 1: no locations (pending).
WARNING:  Unable to resolve breakpoint to any actual locations.
(lldb) run
Process 66372 launched: '/home/bryce/src/apache/arrow/python/venv/bin/python' (x86_64)
Python 3.11.2 (main, Aug 26 2024, 07:20:54) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyarrow as pa
1 location added to breakpoint 1
warning: (x86_64) /home/bryce/src/apache/arrow/python/venv/lib/python3.11/site-packages/numpy.libs/libgfortran-040039e1-0352e75f.so.5.0.0 No LZMA support found for reading .gnu_debugdata section
>>> pa.array([1])
Process 66372 stopped
* thread #1, name = 'python', stop reason = breakpoint 1.1
    frame #0: 0x00007ffff6d6f510 libarrow_python.so`arrow::py::ConvertPySequence(_object*, _object*, arrow::py::PyConversionOptions, arrow::MemoryPool*)
libarrow_python.so`arrow::py::ConvertPySequence:
->  0x7ffff6d6f510 <+0>: pushq  %rbp
    0x7ffff6d6f511 <+1>: movq   %rsp, %rbp
    0x7ffff6d6f514 <+4>: pushq  %r15
    0x7ffff6d6f516 <+6>: movq   %rsi, %r15

One thing I notice is that libarrow_python.so doesn't have debug info like libarrow.so:
(venv) bryce@debian ~/s/a/a/python (main)> file pyarrow/libarrow_python.so
pyarrow/libarrow_python.so: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=be0d7e9dcfba902687a81fec19e5565c4c1d3626, not stripped
(venv) bryce@debian ~/s/a/a/python (main)> file $ARROW_HOME/lib/libarrow.so.1800.0.0
/home/bryce/builds/arrow-x86_64/lib/libarrow.so.1800.0.0: ELF 64-bit LSB shared object, x86-64, version 1 (GNU/Linux), dynamically linked, BuildID[sha1]=282a70ef10aec0f43eb1f10ed176eaa4f993fab2, with debug_info, not stripped
(venv) bryce@debian ~/s/a/a/python (main)>

(In the output above, libarrow_python.so is missing the string "debug_info" in its output).
Source code mapping is working with with symbols in Arrow C++:
(lldb) b Result
Breakpoint 2: 1093 locations.
(lldb) run
There is a running process, kill it and restart?: [Y/n] Y
Process 67013 exited with status = 9 (0x00000009)
Process 67276 launched: '/home/bryce/src/apache/arrow/python/venv/bin/python' (x86_64)
Python 3.11.2 (main, Aug 26 2024, 07:20:54) [GCC 12.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import pyarrow as pa
Process 67276 stopped
* thread #1, name = 'python', stop reason = breakpoint 2.384
    frame #0: 0x00007ffff3d31e12 libarrow.so.1800`arrow::Result<arrow::fs::FileSystemFactoryRegistry::Registered>::Result(this=0x0000000000c7c5e8, value=0x00007fffffffbee0) at result.h:178:29
   175    // NOTE `Result(U&& value)` above should be sufficient, but some compilers
   176    // fail matching it.
   177    Result(T&& value) noexcept {  // NOLINT(runtime/explicit)
-> 178      ConstructValue(std::move(value));
   179    }
   180
   181    /// Copy constructor.

Any ideas @kou?

Could you try PYARROW_BUILD_TYPE=debug environment variable when you build PyArrow?
https://github.com/apache/arrow/blob/0bdb5be03da62de750d3def1a2716536b791b50d/python/setup.py#L170-L171

I did (and generally do). Let me record a full log of a clean build and share it here.

Here's a full log of my install process: https://gist.github.com/amoeba/010c82a7818f8608aef84ca27864c679.

Hrm. Of course it's now working as expected. i.e., I have source code mapping to PyArrow C++ source:
* thread #1, name = 'python', stop reason = breakpoint 1.1
    frame #0: 0x00007ffff6ad8b3b libarrow_python.so`arrow::py::ConvertPySequence(obj=0x00007fffec16c6c0, mask=0x000000000095bcc0, options=PyConversionOptions @ 0x00007fffffffcf80, pool=0x00007ffff631e280) at python_to_arrow.cc:1233:16
   1230 Result<std::shared_ptr<ChunkedArray>> ConvertPySequence(PyObject* obj, PyObject* mask,
   1231                                                         PyConversionOptions options,
   1232                                                         MemoryPool* pool) {
-> 1233   PyAcquireGIL lock;
   1234
   1235   PyObject* seq = nullptr;
   1236   OwnedRef tmp_seq_nanny;

Oh... It's strange... But it's an expected result...

Maybe just caused by not having a totally clean environment. Thanks for the help @kou.
I still wanted to check to see I had the right debug info for the source code mapping the OP was having trouble with and I think I do:
(lldb) image lookup -v -n Authenticate
warning: (x86_64) /home/bryce/src/apache/arrow/python/pyarrow/libarrow_python_flight.so 0x0004c3a5: DW_AT_specification(0x000352af) has no decl
3 matches found in /home/bryce/src/apache/arrow/python/pyarrow/libarrow_python_flight.so:
        Address: libarrow_python_flight.so[0x0000000000027526] (libarrow_python_flight.so.PT_LOAD[1]..text + 1750)
        Summary: libarrow_python_flight.so`arrow::py::flight::PyClientAuthHandler::Authenticate(arrow::flight::ClientAuthSender*, arrow::flight::ClientAuthReader*) at flight.cc:66:85
         Module: file = "/home/bryce/src/apache/arrow/python/pyarrow/libarrow_python_flight.so", arch = "x86_64"
    CompileUnit: id = {0x00000000}, file = "/home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc", language = "c++14"
       Function: id = {0x0004b6c3}, name = "arrow::py::flight::PyClientAuthHandler::Authenticate(arrow::flight::ClientAuthSender*, arrow::flight::ClientAuthReader*)", mangled = "_ZN5arrow2py6flight19PyClientAuthHandler12AuthenticateEPNS_6flight16ClientAuthSenderEPNS3_16ClientAuthReaderE", range = [0x00007fffe7fe6526-0x00007fffe7fe656f)
       FuncType: id = {0x0004b6c3}, byte-size = 0, decl = flight.cc:65:8, compiler_type = "class arrow::Status (class arrow::flight::ClientAuthSender *, class arrow::flight::ClientAuthReader *) const"
         Blocks: id = {0x0004b6c3}, range = [0x7fffe7fe6526-0x7fffe7fe656f)
      **LineEntry: [0x00007fffe7fe6526-0x00007fffe7fe653e): /home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc:66:85**
         Symbol: id = {0x000004ce}, range = [0x00007fffe7fe6526-0x00007fffe7fe656f), name="arrow::py::flight::PyClientAuthHandler::Authenticate(arrow::flight::ClientAuthSender*, arrow::flight::ClientAuthReader*)", mangled="_ZN5arrow2py6flight19PyClientAuthHandler12AuthenticateEPNS_6flight16ClientAuthSenderEPNS3_16ClientAuthReaderE"
       Variable: id = {0x0004b6e2}, name = "this", type = "arrow::py::flight::PyClientAuthHandler *const", location = DW_OP_fbreg -64, decl =
       Variable: id = {0x0004b6ee}, name = "outgoing", type = "arrow::flight::ClientAuthSender *", location = DW_OP_fbreg -72, decl = flight.cc:65:75
       Variable: id = {0x0004b6fe}, name = "incoming", type = "arrow::flight::ClientAuthReader *", location = DW_OP_fbreg -80, decl = flight.cc:66:75
        Address: libarrow_python_flight.so[0x000000000002710e] (libarrow_python_flight.so.PT_LOAD[1]..text + 702)
        Summary: libarrow_python_flight.so`arrow::py::flight::PyServerAuthHandler::Authenticate(arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*) at flight.cc:41:85
         Module: file = "/home/bryce/src/apache/arrow/python/pyarrow/libarrow_python_flight.so", arch = "x86_64"
    CompileUnit: id = {0x00000000}, file = "/home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc", language = "c++14"
       Function: id = {0x0004bbe7}, name = "arrow::py::flight::PyServerAuthHandler::Authenticate(arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*)", mangled = "_ZN5arrow2py6flight19PyServerAuthHandler12AuthenticateEPNS_6flight16ServerAuthSenderEPNS3_16ServerAuthReaderE", range = [0x00007fffe7fe610e-0x00007fffe7fe6157)
       FuncType: id = {0x0004bbe7}, byte-size = 0, decl = flight.cc:40:8, compiler_type = "class arrow::Status (class arrow::flight::ServerAuthSender *, class arrow::flight::ServerAuthReader *) const"
         Blocks: id = {0x0004bbe7}, range = [0x7fffe7fe610e-0x7fffe7fe6157)
      LineEntry: [0x00007fffe7fe610e-0x00007fffe7fe6126): /home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc:41:85
         Symbol: id = {0x0000047e}, range = [0x00007fffe7fe610e-0x00007fffe7fe6157), name="arrow::py::flight::PyServerAuthHandler::Authenticate(arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*)", mangled="_ZN5arrow2py6flight19PyServerAuthHandler12AuthenticateEPNS_6flight16ServerAuthSenderEPNS3_16ServerAuthReaderE"
       Variable: id = {0x0004bc06}, name = "this", type = "arrow::py::flight::PyServerAuthHandler *const", location = DW_OP_fbreg -64, decl =
       Variable: id = {0x0004bc12}, name = "outgoing", type = "arrow::flight::ServerAuthSender *", location = DW_OP_fbreg -72, decl = flight.cc:40:75
       Variable: id = {0x0004bc22}, name = "incoming", type = "arrow::flight::ServerAuthReader *", location = DW_OP_fbreg -80, decl = flight.cc:41:75
        Address: libarrow_python_flight.so[0x000000000002c612] (libarrow_python_flight.so.PT_LOAD[1]..text + 22466)
        Summary: libarrow_python_flight.so`arrow::flight::ServerAuthHandler::Authenticate(arrow::flight::ServerCallContext const&, arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*) at server_auth.h:63:18
         Module: file = "/home/bryce/src/apache/arrow/python/pyarrow/libarrow_python_flight.so", arch = "x86_64"
    CompileUnit: id = {0x00000000}, file = "/home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc", language = "c++14"
       Function: id = {0x0004c3a5}, name = "arrow::flight::ServerAuthHandler::Authenticate(arrow::flight::ServerCallContext const&, arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*)", mangled = "_ZN5arrow6flight17ServerAuthHandler12AuthenticateERKNS0_17ServerCallContextEPNS0_16ServerAuthSenderEPNS0_16ServerAuthReaderE", range = [0x00007fffe7feb612-0x00007fffe7feb658)
       FuncType: id = {0x0004c3a5}, byte-size = 0, decl = server_auth.h:63:18, compiler_type = "class arrow::Status (const class arrow::flight::ServerCallContext &, class arrow::flight::ServerAuthSender *, class arrow::flight::ServerAuthReader *) const"
         Blocks: id = {0x0004c3a5}, range = [0x7fffe7feb612-0x7fffe7feb658)
      LineEntry: [0x00007fffe7feb612-0x00007fffe7feb62e): /home/bryce/builds/arrow-x86_64/include/arrow/flight/server_auth.h:63:18
         Symbol: id = {0x0000026a}, range = [0x00007fffe7feb612-0x00007fffe7feb658), name="arrow::flight::ServerAuthHandler::Authenticate(arrow::flight::ServerCallContext const&, arrow::flight::ServerAuthSender*, arrow::flight::ServerAuthReader*)", mangled="_ZN5arrow6flight17ServerAuthHandler12AuthenticateERKNS0_17ServerCallContextEPNS0_16ServerAuthSenderEPNS0_16ServerAuthReaderE"
       Variable: id = {0x0004c3c4}, name = "this", type = "arrow::flight::ServerAuthHandler *const", location = DW_OP_fbreg -32, decl =
       Variable: id = {0x0004c3d0}, name = "context", type = "const arrow::flight::ServerCallContext &", location = DW_OP_fbreg -40, decl = server_auth.h:63:56
       Variable: id = {0x0004c3df}, name = "outgoing", type = "arrow::flight::ServerAuthSender *", location = DW_OP_fbreg -48, decl = server_auth.h:64:49
       Variable: id = {0x0004c3ee}, name = "incoming", type = "arrow::flight::ServerAuthReader *", location = DW_OP_fbreg -56, decl = server_auth.h:64:77
<<< truncated >>>

Specifically,

LineEntry: [0x00007fffe7fe6526-0x00007fffe7fe653e): /home/bryce/src/apache/arrow/python/pyarrow/src/arrow/python/flight.cc:66:85

So I think that confirms my build is working in a way that would work for the OP's situaton.
@donatobarone I know it's been a while since you were working on this but I think we should be able to help get you set up to debug the issue you were having. I'm going to close this issue for the moment but please comment and we can re-open it.


ignore ._ metadata files created by macos -> linux file sharing

When MacOS shares files to a network drive or removable disk that is formatted differently that doesn't allow some of the metadata fields that MacOS hard drives support you get a bunch of GARBAGE files created in the following filename patterns

.DS_STORE
._<filename>.<ext>

These are storing metadata about the original directory structure and the files themselves... for us the .DS_STORE isn't read, but all these hidden file metadata files are. This leads to the following experience for users.

Some conversation around this issue can be found in the thread here, https://github.com/chiefwigms/picobrew_pico/issues/40

So what is the actual command to ignore the metadata sudo ignore ._ metadata files created by macos -> linux file sharing

@roysbrew there is no "command" just update your server to the latest git sources and these should be ignored by the file glob used by the server to query for recipe files. I've had others with MacOS editing via samba share confirm this is indeed working do you see otherwise after updating? If so what gitsha are you running?

Sorry, what do I need to do to update the server?  Run the update from the Picoserver GUI or is there a SSH command line I need to run?

Either hitting the GUI navigational button as mentioned, powering down and back on, or restarting the "rc.local" service via running sudo systemctl restart rc.local over ssh would all do it.
There are a few other ways too, but those should give you enough options 🤣

Sounds good, I will test and let you know.

Success!  I deleted a recipe and copied the json file back without error.


adding mona dataset

fixes #137
I have uploaded the data to my personal huggingface account, but am happy to move it over to OpenBioML.
I have included the data preprocessing notebook since there is some non-trivial data processing.
I have some ideas on how to set up queries for this dataset, and have provided a few examples in the meta file.

Thanks for your contribution and sorry for letting you wait!
@MicPie can you help with the huggingface?

@adamoyoung Can you also commit the meta.yaml file?
Before we move the HF dataset to OpenBioML we can add the infos from the meta.yaml there.
Let me know if you need help there. 😃

@MicPie great, just to confirm you mean commit the meta.yaml here (and not in the huggingface dataset repo?)

@adamoyoung

great, just to confirm you mean commit the meta.yaml here (and not in the huggingface dataset repo?)
Yes, here in the repo. 👍

@MicPie does this address your comments? I save the dataframe as a csv at data/mona/data_clean.csv. https://github.com/OpenBioML/chemnlp/blob/7e3c48bce214b11c44bda2026ab283766fc28124/data/mona/transform.py#L11

@MicPie is it ready to merge?

Hi @adamoyoung
Again sorry for the late reply, this now looks very good!
The only thing that is not working is that http://semanticscience.org/resource/CHEMINF_000417 does not resolve.
I'm checking that.
If you have time feel free to copy some information from the meta.yaml to the HF dataset card so we have some information there (especially the license). If you are busy, just tell me then I can do that after you moved the dataset.
So just let me know and thank you for your help! :-D


Can you clarify how to refresh and fully reload a wildcards file after changing it?

Describe the bug
Not a bug exactly, more of a question. Sometimes when I make changes to a file in the wildcards directory, the extension doesn't seem to pick up the changes. e.g. I still see it sometimes inserting a tag that I have already removed from the adjectives.txt file when I use __adjectives___ in my prompt (even after a restart of a1111). Please can you clarify:
(a) is there some caching going on, and if so, how can it be cleared?
(b) When are the wildcards files reloaded from disk and when aren't they?
(c) Is there a preferred workflow when updating wildcards files to ensure updates are picked up?
Thanks
To Reproduce
Steps to reproduce:

Create an adjectives.txt file in the wildcards folder with a small number of adjectives.
Use __adjectives__ in a prompt.
Start generating a batch of images with that prompt.
Remove an adjective from the file and save it.
(optionally) restart a1111 (or try other things to clear cache)
Repeat steps 2 and 3, generate a large number of images.
Inspect the PNG info or generated prompt files - Notice that sometimes the deleted adjective will still be used in some image generations/prompts.

Settings: extension enabled, standard wildcard settings selected, not using combinatorial batches, no magic prompt stuff.
Are you using a local install or a colab?
local
Expected behaviour

the removal of a wildcard should be picked up by the extension at a definable moment/event, and that removed entry should never show up any more in prompts after that moment.
the user should be able to know with certainty when an updated wildcards file will be picked up and when it won't.

Additional context
N/A

@adieyal By the way, something I've been thinking about that it could be neat to store wildcards in an SQLite database. I think it would be pretty cool to be able to quickly search for a wildcard "file" based on words contained within...

I've been thinking the same thing, a search box in the Wildcards Manager tab to quickly find a wildcard would be helpful.

thanks for the quick response. Incidentally I found the problem was partly because I had accidentally got a word in more than one file, so it was still picking it up from the other file even after cache clearing! sorry about that, it's probably working better than I thought!


Error when generate: last_commit is nil: GitStats::GitData::Repo

within the repository by executing:: git_stats generate
fatal: |: no such path in the working tree.
Use 'git <command> -- <path>...' to specify paths that do not exist locally.
C:in `rescue in files_by_extension_count': GitStats::GitData::Repo#files_by_extension_count delegated to last_commit.files_by_extension_count, but last_commit is nil: GitStats::GitData::Repo C:/Users/Riderman/Documents/..MyRepo.. (Module::DelegationError)
        from C:in `files_by_extension_count'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/charts/repo_charts.rb:13:in `block in files_by_extension'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/charts/chart.rb:13:in `initialize'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/charts/repo_charts.rb:11:in `new'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/charts/repo_charts.rb:11:in `files_by_extension'
        from C:in `files_by_extension'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/_files.haml:15:in `block in singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/_files.haml:65527:in `instance_eval'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/_files.haml:65527:in `singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/_files.haml:65525:in `__tilt_15822660'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `call'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/haml.rb:17:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:96:in `render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/template.rb:15:in `render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/view_data.rb:18:in `render_partial'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/by_extension.haml:1:in `block in singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/by_extension.haml:65526:in `instance_eval'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/by_extension.haml:65526:in `singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/files/by_extension.haml:65524:in `__tilt_15822660'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `call'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/haml.rb:17:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:96:in `render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/template.rb:13:in `block in render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/layout.haml:30:in `block in singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/layout.haml:65526:in `instance_eval'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/layout.haml:65526:in `singletonclass'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/templates/layout.haml:65524:in `__tilt_15822660'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `call'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:155:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/haml.rb:17:in `evaluate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/tilt-2.0.1/lib/tilt/template.rb:96:in `render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/template.rb:13:in `render'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/view.rb:15:in `block in render_all'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/view.rb:14:in `each'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/stats_view/view.rb:14:in `render_all'
        from C:in `render_all'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/lib/git_stats/cli.rb:18:in `generate'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/thor-0.19.1/lib/thor/command.rb:27:in `run'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/thor-0.19.1/lib/thor/invocation.rb:126:in `invoke_command'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/thor-0.19.1/lib/thor.rb:359:in `dispatch'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/thor-0.19.1/lib/thor/base.rb:440:in `start'
        from C:/ruby193/lib/ruby/gems/1.9.1/gems/git_stats-1.0.14/bin/git_stats:10:in `<top (required)>'
        from C:/ruby193/bin/git_stats:23:in `load'
        from C:/ruby193/bin/git_stats:23:in `<main>'

Same error.... :(

Got same error

Another one :(

Também estou com o mesmo erro!, alguma previsão para ajustar?
Seria somente no windows?

Same problem here. To provide some additional info, I installed git_stats using chocolatey. It does not matter how big or small the git repo. After it didn't work on my very large repo, I also tried it on a repo with just one file and one commit.

Same problem here. To provide some additional info, I installed git_stats using chocolatey. It does not matter how big or small the git repo. After it didn't work on my very large repo, I also tried it on a repo with just one file and one commit.

same error.. did this solved?

same error :(


Confusion about rowHasChanged and ListView rendering

Hey all,
While working on some performance improvements I found out that rowHasChanged was not called in any of our lists. These lists are infinite-scrolling lists, meaning that when you reach the end, it will fetch more items to display and then dump them into the list.
It seems like all entries in the list are updated whenever we change the DataSource's content. We do have a shouldComponentUpdate in place for all rows, but from how I understood rowHasChanged, these rows would not needed to be rendered again.
I created a simple example that shows how we are appending items to the list: https://gist.github.com/janmonschke/c9c84b6050683da0c64f
What we basically do is:
constructor() {
  // (...)
  this.ds = new ListView.DataSource({rowHasChanged: (r1, r2) => {
    console.log('rowHasChanged');
    return r1 !== r2;
  }});
  // (...)
}

onEndReached = () => {
  // append data to the end
  setData = [...setData, ...genRandomSet()];
  this.setState({
    dataSource: this.ds.cloneWithRows(setData)
  });
  console.log('onEndReached');
};

render() {
  return (
    <ListView
      dataSource={this.state.dataSource}
      renderRow={this.renderRow}
      onEndReached={this.onEndReached}
    />
  );
}

But rowHasChanged is never logged.
Am I using it somehow in a wrong way? Are there better ways to append to a ListView?
Same behaviour in 0.18.0 and 0.19.0

Okay, found the solution in: http://stackoverflow.com/a/33871118.
Apparently it's important to clone from the correct DataSource instance ;)
So the code from the gist should have been:
  setData = [...setData, ...genRandomSet()];
  this.setState({
    dataSource: this.state.dataSource.cloneWithRows(setData)
  });

This seems like an error that's easily sneaking into your codebase. I wonder if the correct usage of DataSource should be more emphasised in the documentation.

Thanks @janmonschke! I agree, I had a bunch of views constantly re-rendering and I couldn't figure out why that was happening and why the rowHasChanged function was never getting called. 👊


[SPARK-8735] [SQL] Expose memory usage for shuffles, joins and aggregations

This patch exposes the memory used by internal data structures on the SparkUI. This tracks memory used by all spilling operations and SQL operators backed by Tungsten, e.g. BroadcastHashJoin, ExternalSort, GeneratedAggregate etc. The metric exposed is "peak execution memory", which broadly refers to the peak in-memory sizes of each of these data structure.
A separate patch will extend this by linking the new information to the SQL operators themselves.

Merged build triggered.

Merged build started.

Test build #38958 has started for   PR 7770 at commit 5b5e6f3.

Test build #38958 has finished for   PR 7770 at commit 5b5e6f3.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

The information exposed in this patch will be tied to accumulators on the SQL tab introduced in #7774

Merged build triggered.

Merged build started.

Test build #1253 has started for   PR 7770 at commit 0086500.

Test build #1254 has started for   PR 7770 at commit 0086500.

Merged build triggered.

Merged build started.

Test build #39169 has started for   PR 7770 at commit 111a05e.

Merged build finished. Test FAILed.

Test build #1253 timed out     for PR 7770 at commit 0086500     after a configured wait of 175m.

Test build #1254 timed out     for PR 7770 at commit 0086500     after a configured wait of 175m.

Test build #39169 timed out     for PR 7770 at commit 111a05e     after a configured wait of 175m.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #39243 has started for   PR 7770 at commit 0b6926c.

Merged build triggered.

Merged build started.

Test build #39245 has started for   PR 7770 at commit 5f1235b.

Test build #39245 has finished for   PR 7770 at commit 5f1235b.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
 public static final class StringPrefixComparatorDesc extends PrefixComparator
 public static final class LongPrefixComparator extends PrefixComparator
 public static final class LongPrefixComparatorDesc extends PrefixComparator
 public static final class DoublePrefixComparator extends PrefixComparator
 public static final class DoublePrefixComparatorDesc extends PrefixComparator
class MulticlassClassificationEvaluator (override val uid: String)
class NaiveBayes(JavaEstimator, HasFeaturesCol, HasLabelCol, HasPredictionCol):
class NaiveBayesModel(JavaModel):
class MulticlassClassificationEvaluator(JavaEvaluator, HasLabelCol, HasPredictionCol):
 public static class CompactDecimalWriter
 public static class DecimalWriter
abstract class TernaryExpression extends Expression
case class SortPrefix(child: SortOrder) extends UnaryExpression
case class ToDate(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
case class TruncDate(date: Expression, format: Expression)
case class TungstenSort(

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #39263 has started for   PR 7770 at commit 2480d84.

Test build #39263 has finished for   PR 7770 at commit 2480d84.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class TernaryExpression extends Expression

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #39264 has started for   PR 7770 at commit d090a94.

Test build #1262 has started for   PR 7770 at commit d090a94.

Test build #1263 has started for   PR 7770 at commit d090a94.

Test build #39243 timed out     for PR 7770 at commit 0b6926c     after a configured wait of 175m.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #39271 has started for   PR 7770 at commit 9b86245.

Merged build triggered.

Merged build started.

Test build #39271 has finished for   PR 7770 at commit 9b86245.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

retest this please

Test build #39272 has started for   PR 7770 at commit b889a68.

Merged build triggered.

Merged build started.

Test build #39273 has started for   PR 7770 at commit 6aa2f7a.

I admit I need more time to understand the code changes, but I'm excited about this addition, it'll be a great feature to help users understand what is going on!

Test build #39264 timed out     for PR 7770 at commit d090a94     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test build #1262 timed out     for PR 7770 at commit d090a94     after a configured wait of 175m.

Test build #1263 timed out     for PR 7770 at commit d090a94     after a configured wait of 175m.

Test build #39272 timed out     for PR 7770 at commit b889a68     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test build #39273 timed out     for PR 7770 at commit 6aa2f7a     after a configured wait of 175m.

Merged build finished. Test FAILed.

It looks like this has become merge-conflicted again, likely due to the rapid pace of changes in the Tungsten code. In order to avoid merge-conflict hell, it might be a good idea to try to get this patch into a MVP state, merge it, and defer followup work to a series of smaller patches.

To help me cope with the large size of this patch, I'm going to try to use Reviewable to manage my initial round of comments: https://reviewable.io/reviews/apache/spark/7770

I took a quick pass through the current diff.
One high-level question:
The tooltip comment says that this will only be used if Tungsten is enabled, but I noticed that there are also peak memory consumption tests for several non-Tungsten operators, including the existing external sorter, ExternalAppendOnlyMap, etc.  Is the idea that displaying only those operators' memory usage for non-Tungsten jobs will be confusing? It seems like the same confusion could arise if a user has Tungsten enabled but is running non-SQL jobs. Given this, I wonder whether it makes sense to just always show that metric irrespective of whether Tungsten is used.

Reviewed 7 of 35 files at r1, 8 of 11 files at r2, 2 of 4 files at r3, 4 of 24 files at r4, 1 of 1 files at r5, 4 of 10 files at r6, 1 of 13 files at r7, 3 of 4 files at r9, 1 of 3 files at r10.
Review status: 31 of 49 files reviewed at latest revision, 17 unresolved discussions, some commit checks failed.

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 455 [r11] (raw file):
This could possibly be null due to mocking.  Do you remember which tests this was null in?

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 459 [r11] (raw file):
Why is this Java conversion necessary?  As far as I know, you should still be able to call methods in Scala maps from Java, although you might have some weird looking imports.

core/src/main/scala/org/apache/spark/Accumulators.scala, line 157 [r11] (raw file):
I fear that this could mask bugs if TaskContext is null when we're trying to deserialize an external accumulator.  Instead of doing a null check here, could you write out the isInternal flag and check that here to decide whether to register? If you do that, can you also add a comment that acts as a cross-reference to explain where internal accumulators are registered?

core/src/main/scala/org/apache/spark/Accumulators.scala, line 264 [r11] (raw file):
Name boolean parameters?  IntelliJ likes to complain about this.

core/src/main/scala/org/apache/spark/Accumulators.scala, line 268 [r11] (raw file):
Name boolean parameters?

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala, line 791 [r1] (raw file):
Will this change break any user programs that may have relied on the old behavior? Was the old behavior specified?

core/src/main/scala/org/apache/spark/scheduler/Stage.scala, line 78 [r11] (raw file):
Should this comment describe what happens during partial stage recomputations?

core/src/main/scala/org/apache/spark/TaskContext.scala, line 65 [r11] (raw file):
Could use a @VisibleForTesting annotation here.

core/src/main/scala/org/apache/spark/TaskContext.scala, line 67 [r11] (raw file):
I thought that you could call private[spark] and protected[spark] methods from Java?

core/src/test/scala/org/apache/spark/CacheManagerSuite.scala, line 89 [r11] (raw file):
Given that we removed local execution in 1.5, we might be able to remove this code as well.  Shouldn't happen here, but just wanted to note it since I just noticed this.

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala, line 1621 [r11] (raw file):
Instead of using originalValue and a finally, block, this can be slightly simplified by using the new withSQLConf helper methods from SQLTestUtils (which is mixed into this suite).  Take a look at other uses in this file; should be straightforward cleanup.

unsafe/src/main/java/org/apache/spark/unsafe/map/BytesToBytesMap.java, line 343 [r11] (raw file):
This change is conflicted due to refactoring in Reynold's latest patch.

Comments from the review on Reviewable.io

Review status: 31 of 49 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleExternalSorter.java, line 89 [r1] (raw file):
fixed.

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 455 [r11] (raw file):
UnsafeShuffleWriterSuite. I can add that to the comment

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 459 [r11] (raw file):
It didn't work when I tried, but I can see if there's a way to make it work again

core/src/main/scala/org/apache/spark/Accumulators.scala, line 157 [r11] (raw file):
sounds good

core/src/main/scala/org/apache/spark/Accumulators.scala, line 264 [r11] (raw file):
ok

core/src/main/scala/org/apache/spark/Accumulators.scala, line 268 [r11] (raw file):
k

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala, line 791 [r1] (raw file):
This only deals with internal accumulators, which are new, right? What old behavior are you referring to?

core/src/main/scala/org/apache/spark/scheduler/Stage.scala, line 78 [r11] (raw file):
sure

core/src/main/scala/org/apache/spark/TaskContext.scala, line 65 [r11] (raw file):
ok

core/src/main/scala/org/apache/spark/TaskContext.scala, line 67 [r11] (raw file):
I tried it. I couldn't get it to compile. It was complaining that TaskContext has no member empty() or something.

core/src/test/scala/org/apache/spark/CacheManagerSuite.scala, line 89 [r11] (raw file):
k, let's do that outside of this patch.

sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeFixedWidthAggregationMap.java, line 206 [r1] (raw file):
Done.

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala, line 1621 [r11] (raw file):
sure

Comments from the review on Reviewable.io

Review status: 31 of 49 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 455 [r11] (raw file):
Done.

core/src/main/java/org/apache/spark/shuffle/unsafe/UnsafeShuffleWriter.java, line 459 [r11] (raw file):
you're right, I removed this conversion

core/src/main/scala/org/apache/spark/Accumulators.scala, line 157 [r11] (raw file):
Done.

core/src/main/scala/org/apache/spark/Accumulators.scala, line 264 [r11] (raw file):
actually it would not compile, see comment below

core/src/main/scala/org/apache/spark/Accumulators.scala, line 268 [r11] (raw file):
It turns out scala thinks I'm reassigning a val if I do internal = false here.

core/src/main/scala/org/apache/spark/scheduler/Stage.scala, line 78 [r11] (raw file):
Done.

core/src/main/scala/org/apache/spark/TaskContext.scala, line 65 [r11] (raw file):
Done.

core/src/main/scala/org/apache/spark/TaskContext.scala, line 67 [r11] (raw file):
Weird, it went away. I made this private[spark]

core/src/main/scala/org/apache/spark/ui/jobs/StagePage.scala, line 72 [r11] (raw file):
The issue is that we don't track all SQL operators (e.g. normal non-unsafe in-memory Sort). It would be misleading if Sort ends up having lower "peak execution memory" than UnsafeExternalSort. Let's move this discussion to the main thread.

core/src/main/scala/org/apache/spark/ui/ToolTips.scala, line 70 [r11] (raw file):
There is not a great name for "aggregation + shuffle + join". Elsewhere in Spark we actually incorrectly use "spark.shuffle.memoryFraction" for non-shuffle operations, simply because there is not a more general category. I'm not a die-hard fan of "execution memory" either, but it's the best I can come up with.

sql/core/src/main/scala/org/apache/spark/sql/execution/sort.scala, line 81 [r1] (raw file):
Done.

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala, line 1621 [r11] (raw file):
Done.

Comments from the review on Reviewable.io

Merged build triggered.

Merged build started.

Test build #39450 has started for   PR 7770 at commit d70874d.

It seems the main source of outstanding debate is whether to display this metric only when unsafe is enabled, which is the proposal of this patch.
The motivation is as follows: there are SQL operators whose memory usage is both high and difficult to track without sacrificing performance (e.g. Sort simply uses Arrays.sort, or BroadcastHashJoin with GeneralHashedRelation uses a Java hash map under the hood). It would be misleading if we say the memory usage in these operators is low simply because we do not track them.
There are two ways of fixing this: (1) only display this when unsafe is enabled, and (2) detect cases when jobs are triggered due to the operators we don't track. (2) is of a significantly wider scope than the intention of this patch, so we just went with (1) here. Note that this means we don't expose this metric even for simple non-SQL aggregations (e.g. reduceByKey), but working around that would require us to detect whether a job is a SQL job, which is being implemented in #7774 but outside the scope of this individual patch.

My view is that we can always enable it for more general cases in the future, but going the other way is technically a regression.

retest this please

Test build #1283 has started for   PR 7770 at commit d70874d.

Test build #39450 has finished for   PR 7770 at commit d70874d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

Test build #39456 has started for   PR 7770 at commit a87b4d0.

Test build #1283 has finished for   PR 7770 at commit d70874d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
  case class RequestExecutors(appId: String, requestedTotal: Int)
  case class KillExecutors(appId: String, executorIds: Seq[String])
public class JavaKMeansExample
class StopWordsRemover(override val uid: String)
 class FreqSequence[Item](val sequence: Array[Array[Item]], val freq: Long) extends Serializable
class PrefixSpanModel[Item](val freqSequences: RDD[PrefixSpan.FreqSequence[Item]])
      class SpecificSafeProjection extends $
case class SortArray(base: Expression, ascendingOrder: Expression)
case class FromUTCTimestamp(left: Expression, right: Expression)
case class ToUTCTimestamp(left: Expression, right: Expression)
case class DateDiff(endDate: Expression, startDate: Expression)
case class InitCap(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
public final class UnsafeKVExternalSorter

Weird failures! retest this please?

Merged build triggered.

Merged build started.

Test build #39463 has started for   PR 7770 at commit a87b4d0.

Test build #39456 has finished for   PR 7770 at commit a87b4d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
 class FreqSequence[Item](val sequence: Array[Array[Item]], val freq: Long) extends Serializable
class PrefixSpanModel[Item](val freqSequences: RDD[PrefixSpan.FreqSequence[Item]])
public final class UnsafeKVExternalSorter

Merged build finished. Test FAILed.

Test build #1287 has started for   PR 7770 at commit a87b4d0.

Test build #1288 has started for   PR 7770 at commit a87b4d0.

Test build #39463 has finished for   PR 7770 at commit a87b4d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Merged build triggered.

Merged build started.

[error] /home/jenkins/workspace/SparkPullRequestBuilder@3/core/src/test/java/org/apache/spark/JavaAPISuite.java:1013: error: cannot find symbol
[error]     TaskContext context = TaskContext.empty();
[error]                                      ^
[error]   symbol:   method empty()
[error]   location: class TaskContext

I think that you need to import TaskContext$ so that you get this field from the companion object.

Test build #39469 has started for   PR 7770 at commit 17f4c2d.

Test build #1289 has started for   PR 7770 at commit 17f4c2d.

Test build #1287 has finished for   PR 7770 at commit a87b4d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
  case class RequestExecutors(appId: String, requestedTotal: Int)
  case class KillExecutors(appId: String, executorIds: Seq[String])
public class JavaKMeansExample
 class FreqSequence[Item](val sequence: Array[Array[Item]], val freq: Long) extends Serializable
class PrefixSpanModel[Item](val freqSequences: RDD[PrefixSpan.FreqSequence[Item]])
      class SpecificSafeProjection extends $
case class FromUTCTimestamp(left: Expression, right: Expression)
case class ToUTCTimestamp(left: Expression, right: Expression)
case class DateDiff(endDate: Expression, startDate: Expression)
case class InitCap(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
public final class UnsafeKVExternalSorter

Test build #1288 has finished for   PR 7770 at commit a87b4d0.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

I think that you need to import TaskContext$ so that you get this field from the companion object.

but JavaAPISuite is in org.apache.spark so it should already import it. When I added this my IDE told me the import is not used.

Merged build triggered.

Merged build started.

Test build #39477 has started for   PR 7770 at commit 10da1cd.

retest this please

Merged build triggered.

Merged build started.

Test build #39479 has started for   PR 7770 at commit c00a197.

Test build #1290 has started for   PR 7770 at commit c00a197.

Test build #1291 has started for   PR 7770 at commit c00a197.

Test build #1292 has started for   PR 7770 at commit c00a197.

Merged build triggered.

Merged build started.

Test build #39484 has started for   PR 7770 at commit 0625d73.

Test build #39469 has finished for   PR 7770 at commit 17f4c2d.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Reviewed 1 of 24 files at r4, 2 of 10 files at r6, 5 of 13 files at r7, 1 of 1 files at r8, 1 of 4 files at r9, 1 of 3 files at r10, 16 of 20 files at r12, 2 of 2 files at r15, 3 of 3 files at r16.
Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala, line 791 [r1] (raw file):
Ah, right.  SGTM.

core/src/test/scala/org/apache/spark/AccumulatorSuite.scala, line 275 [r16] (raw file):
Could also just throw an exception here.

core/src/test/scala/org/apache/spark/ui/StagePageSuite.scala, line 53 [r13] (raw file):
Minor Mockito trick that I like using: the second constructor argument to Mock specifies the default answer returned for non-mocked methods.  If you pass Mockito.RETURNS_SMART_NULLS as the second argument then you'll get error messages which provide much more detail if an NPE occurs due to a non mocked / stubbed method.

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala, line 408 [r16] (raw file):
This test seems like it might become slightly brittle in the future.  To guard against this, it might be a nice idea to integrate with the listener / metrics systems to verify that spilling did/didn't occur.  It's fine to leave this to a followup patch, though, given that this is not a huge concern.

sql/core/src/main/scala/org/apache/spark/sql/execution/GeneratedAggregate.scala, line 306 [r16] (raw file):
I think that we should move this call up slightly so that it takes place right before we return this result iterator.  This will prevent this memory accounting from under-reporting peak memory in case we decide to make aggregationMap.iterator() into a destructive iterator which frees pages as it returns records.

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/BroadcastHashJoin.scala, line 78 [r16] (raw file):
One thought: a broadcast variable is used by multiple tasks but it will only be created once per JVM, so I think that this will slightly overcount the broadcast variable's resource usage.  For instance, with the current accounting: if you have a four-core machine with 1 gigabyte of RAM per core and a 1.5 gigabyte broadcast variable then you might see a peak execution memory of 4 * 1.5 = 6 gb, which is more than the total RAM, but the actual memory consumption wasn't actually this high.
One way to deal with this would be to split a task's memory consumption into two categories, private and shared, similar to how operating systems' top tools (or Activity Monitor in OSX) report shared memory separately from applications' private memory.  This change is pretty easy to make and I don't think that it needs to be done as part of this patch.

sql/core/src/main/scala/org/apache/spark/sql/execution/sort.scala, line 81 [r1] (raw file):
I'm pretty sure that .iterator() can only be called once (given that it ends up calling methods with names like destructiveSorted...), so I think that we should add some asserts to enforce this implicit contract then should move this metrics updating code into the ExternalSorter itself.  Pushing the spill metrics updates closer to the site of the spilling itself will make it easier to keep the metrics in sync with the underlying writes.

sql/core/src/test/scala/org/apache/spark/sql/BroadcastJoinSuite.scala, line 18 [r16] (raw file):
Minor nit: since this is testing something in the sql.executions.joins_ package, the test should probably be in that package as well.

sql/core/src/test/scala/org/apache/spark/sql/BroadcastJoinSuite.scala, line 49 [r16] (raw file):
I'm fairly sure that this is going to lead to a "multiple active SparkContexts in the same JVM" failure, since TestSQLContext might have already created an active global context before this runs.  Fixing this might require some fairly large refactorings to TestSQLContext and our other testing code, so for now we may end up having to disable this test.  If we need to do that, let's file a 1.5.0 test blocker JIRA.

Comments from the review on Reviewable.io

This looks good to me overall. I think that there are a couple of small tasks that we might want to defer to smaller followup patches:

Figuring out whether we want to account for broadcast variables via a separate "shared task memory" metric.
If necessary, blocking one of the SQL tests on cleanup of TestSQLContext (this can happen during the QA period).

The only minor blockers at this point are some slight fixes to where we update metrics in order to guard against breakage due to future changes (e.g. ensure that the update will still be correct if a non-destructive iterator was updated to be destructive).

It looks like this is currently failing a bunch of different tests due to a NPE in TorrentBroadcast:
[info] - function current_date *** FAILED *** (20 milliseconds)
[info]   org.apache.spark.SparkException: Job aborted due to stage failure: Task serialization failed: java.lang.NullPointerException
[info] org.apache.spark.broadcast.TorrentBroadcast.<init>(TorrentBroadcast.scala:80)
[info] org.apache.spark.broadcast.TorrentBroadcastFactory.newBroadcast(TorrentBroadcastFactory.scala:34)
[info] org.apache.spark.broadcast.BroadcastManager.newBroadcast(BroadcastManager.scala:63)
[info] org.apache.spark.SparkContext.broadcast(SparkContext.scala:1295)
[info] org.apache.spark.scheduler.DAGScheduler.org$apache$spark$scheduler$DAGScheduler$$submitMissingTasks(DAGScheduler.scala:844)
[info] org.apache.spark.scheduler.DAGScheduler.org$apache$spark$scheduler$DAGScheduler$$submitStage(DAGScheduler.scala:756)
[info] org.apache.spark.scheduler.DAGScheduler.handleJobSubmitted(DAGScheduler.scala:741)
[info] org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.doOnReceive(DAGScheduler.scala:1436)
[info] org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.onReceive(DAGScheduler.scala:1428)
[info] org.apache.spark.scheduler.DAGSchedulerEventProcessLoop.onReceive(DAGScheduler.scala:1417)
[info] org.apache.spark.util.EventLoop$$anon$1.run(EventLoop.scala:48)

It looks like that line of TorrentBroadcast could be failing on SparkEnv.get(), which is a little surprising given that SparkEnv is now effectively a global rather than a thread-local.

Review status: 49 of 51 files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

core/src/test/scala/org/apache/spark/AccumulatorSuite.scala, line 275 [r16] (raw file):
Done.

core/src/test/scala/org/apache/spark/ui/StagePageSuite.scala, line 53 [r13] (raw file):
Done.

core/src/test/scala/org/apache/spark/util/collection/ExternalAppendOnlyMapSuite.scala, line 408 [r16] (raw file):
Ok, later

sql/core/src/main/scala/org/apache/spark/sql/execution/GeneratedAggregate.scala, line 306 [r16] (raw file):
Done.

sql/core/src/main/scala/org/apache/spark/sql/execution/joins/BroadcastHashJoin.scala, line 78 [r16] (raw file):
ok, we'll do that outside this patch

sql/core/src/main/scala/org/apache/spark/sql/execution/sort.scala, line 81 [r1] (raw file):
ok, we'll do that outside this patch

sql/core/src/test/scala/org/apache/spark/sql/BroadcastJoinSuite.scala, line 18 [r16] (raw file):
ah ok

sql/core/src/test/scala/org/apache/spark/sql/BroadcastJoinSuite.scala, line 49 [r16] (raw file):
OK, if it doesn't pass already then it has to be a blocker. Otherwise let's leave it till later.

Comments from the review on Reviewable.io

Merged build triggered.

Merged build started.

Alright, I'm guessing the test explosion is caused by the fact that we have multiple SparkContexts in the same JVM during the SQL tests. Let's see if the latest commit fixes this. If so, we'll have to find a way to uncomment the test later before the 1.5 release.

Test build #1293 has started for   PR 7770 at commit d0fef87.

Test build #1294 has started for   PR 7770 at commit d0fef87.

Test build #1295 has started for   PR 7770 at commit d0fef87.

Merged build triggered.

Merged build started.

Test build #1293 has finished for   PR 7770 at commit d0fef87.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1294 has finished for   PR 7770 at commit d0fef87.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1295 has finished for   PR 7770 at commit d0fef87.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
  case class RequestExecutors(appId: String, requestedTotal: Int)
  case class KillExecutors(appId: String, executorIds: Seq[String])
public class JavaKMeansExample
class StopWordsRemover(override val uid: String)
 class FreqSequence[Item](val sequence: Array[Array[Item]], val freq: Long) extends Serializable
class PrefixSpanModel[Item](val freqSequences: RDD[PrefixSpan.FreqSequence[Item]])
      class SpecificSafeProjection extends $
case class SortArray(base: Expression, ascendingOrder: Expression)
case class FromUTCTimestamp(left: Expression, right: Expression)
case class ToUTCTimestamp(left: Expression, right: Expression)
case class DateDiff(endDate: Expression, startDate: Expression)
case class InitCap(child: Expression) extends UnaryExpression with ImplicitCastInputTypes
public final class UnsafeKVExternalSorter

Haha, looks like I was wrong about commenting code out like that :(

Test build #1296 has started for   PR 7770 at commit d0fef87.

Test build #1297 has started for   PR 7770 at commit d0fef87.

Test build #1298 has started for   PR 7770 at commit d0fef87.

Merged build triggered.

Merged build started.

Test build #1289 timed out     for PR 7770 at commit 17f4c2d     after a configured wait of 175m.

Test build #39494 has started for   PR 7770 at commit 40b4802.

Merged build finished. Test FAILed.

Merged build finished. Test FAILed.

Test build #1290 has finished for   PR 7770 at commit c00a197.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1292 has finished for   PR 7770 at commit c00a197.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #39477 timed out     for PR 7770 at commit 10da1cd     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test build #39479 timed out     for PR 7770 at commit c00a197     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test build #1291 timed out     for PR 7770 at commit c00a197     after a configured wait of 175m.

Whats the reason for using accumulators instead of TaskMetrics?

Test build #39484 timed out     for PR 7770 at commit 0625d73     after a configured wait of 175m.

Merged build finished. Test FAILed.

Test build #1296 has finished for   PR 7770 at commit d0fef87.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #39494 has finished for   PR 7770 at commit 40b4802.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test build #1297 has finished for   PR 7770 at commit d0fef87.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1298 has finished for   PR 7770 at commit d0fef87.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

It looks like this is now passing all tests except for one of its own, BytesToBytesMapSuite.testTotalMemoryConsumption.

@squito regarding accumulators, there was an earlier patch that supports incremental updates of accumulators, so we can use them for this type of measurement now. Over time IMO we should move most of the metrics into accumulators since that's a built in mechanism for doing exactly this type of measurement, and involves fewer hard-coded changes when we want to add new instrumentation. TaskMetrics was sort of a parallel feature that developed but now has almost 100% overlap with the accumulators concept.

I was able to reproduce the test failure locally and fixed it. There were many logical merge conflicts that changed the underlying assumptions of a few of my tests. This has happened a few times already and it would be good to get this merged soon. Once Jenkins wakes up, that is...

Merged build triggered.

To add to @pwendell's comment, the biggest advantage of accumulators is the ability to deal with failed and resubmitted tasks. Double counting has been a perennial issue for consuming data encoded in task metrics through the listener interface. For this reason I think it's good to eventually migrate everything to accumulators, which is also more general and has little impact on backwards compatibility.

Merged build started.

Test build #1305 has started for   PR 7770 at commit 9de2a12.

Test build #39539 has started for   PR 7770 at commit 9de2a12.

I didn't realize the point of that other PR was so we could start switching all task metrics over ... accumulators have their own set of confusing semantics around retries.  Plus You lose all type info on the values, now they come through as just strings, and also the "key" goes from being a member of the developer api that you can check against, to being a string constant that is private[spark].
I don't see how we will migrate without breaking backwards compatibility.  It seems like we'll just be stuck with some metrics in each until we go to 2.0.  At that point we might be better off redesigning a metrics system with the semantics we actually want.
I can see the appeal of adding some untyped way to get metrics, like MR counters, which even users could add to, I just don't know if accumulators actually give it to us.  But it sounds like you are telling me this is already a fait accompli?

Test build #39539 has finished for   PR 7770 at commit 9de2a12.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test FAILed.

Test build #1305 has finished for   PR 7770 at commit 9de2a12.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

@squito we won't remove TaskMetrics before 2.0 because it breaks compatibility, as you mentioned. Also, in my mind the semantics of accumulators during retries are fairly clear actually; if a task fails, its accumulator values are not counted, whereas with TaskMetrics you have to check the task end status in all the downstream listeners, thus requiring code duplication for correctness.
I agree that accumulators are not perfect. The fact that there is a singleton Accumulator object per JVM is a little strange, for instance. For a clean redesign in 2.0 ideally we can clean up a lot of the remaining sources of confusion with accumulators and unify it with TaskMetrics somehow so we get the best of both worlds (e.g. provide type safety). Until then, I personally find accumulators easier to work with.

To clarify slightly, (@andrewor14, correct me if I'm wrong), these new internal metrics are considered to be completely private[spark] right now, so there's no @DeveloperApi guarantees for them yet.  As a result, I'm not sure that we necessarily have compatibility concerns here since we don't expose / define any programmatic APIs around these metrics yet (in other words, I think the use of accumulators is just an implementation detail).

Reviewed 2 of 2 files at r17, 3 of 5 files at r18, 1 of 1 files at r20, 4 of 4 files at r21.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

LGTM pending Jenkins.

Merged build triggered.

Merged build started.

Test build #1307 has started for   PR 7770 at commit d7df332.

Test build #1309 has started for   PR 7770 at commit d7df332.

Test build #1308 has started for   PR 7770 at commit d7df332.

Test build #1310 has started for   PR 7770 at commit d7df332.

Merged build triggered.

Test build #1307 has finished for   PR 7770 at commit d7df332.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

Merged build started.

Test build #39568 has started for   PR 7770 at commit f5b0d68.

Test build #1311 has started for   PR 7770 at commit d7df332.

Test build #1312 has started for   PR 7770 at commit d7df332.

Test build #1313 has started for   PR 7770 at commit f5b0d68.

Merged build finished. Test FAILed.

Test build #1314 has started for   PR 7770 at commit f5b0d68.

Test build #1316 has started for   PR 7770 at commit f5b0d68.

Test build #1315 has started for   PR 7770 at commit f5b0d68.

Merged build triggered.

Merged build started.

Test build #39571 has started for   PR 7770 at commit 9abecb9.

Accumulators handle task failures, but have super confusing semantics around recomputation from shared lineage, speculative execution, and stage retries.  (eg., I don't understand the current logic for clearing these new values).
Making this an internal accumulator is certainly "safe" in that its not technically exposed at all, so we could change it without breaking any contract at all.  I just meant that we seem to be headed to a state where (a) any user SparkListener won't have any access to this, other than just hardcoding a check for "peakExecutionMemory" into their code, which could break at any time.  So a user spark listener "shouldn't" do that, but we do expose other UI info that way so its weird for this one tiny bit to be hidden.  And I'm saying that in general, we seem to be headed to a future where a user SparkListener will get some set of metrics from the strongly typed and developer api TaskMetrics, and then they'll just snoop our internal accumulators for other stuff.
In any case, I guess I've said my piece and I'm not convincing anybody.

Hey Imran,
So I think there is a larger design discussion at hand that we can probably break out into a mailing list thread or maybe discuss offline - about as we add more metrics what internal mechanisms do we use and how to we present them to the user (these could be decoupled as concerns, actually). We had been going down the direction of moving some of these to accumulators to avoid some of the clunkiness of the taskmetrics approach and to avoid duplicate mechanism to the same things (we discussed along these lines even when doing updates to taskmetrics way back with @sryza).
However, an open question is how will this affect the user-facing story regarding consumption. For instance, we could have public constants for accumulator keys we plan to support and give them standard naming, if we want them to be caught by MIMA and more formally supported.
I would recommend forking a new thread on the dev list to discuss the trade-off's. This current patch as it stands, I don't think it represents a long term commitment in any direction. We could always add these to the TaskMetrics struct if we wanted to later on send them back to users in that way. We could also have all the TaskMetrics use accumulators internally and still support the current user-facing struct classes.

Test build #1312 has finished for   PR 7770 at commit d7df332.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

Test build #39568 has finished for   PR 7770 at commit f5b0d68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Test build #1315 has finished for   PR 7770 at commit f5b0d68.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1311 has finished for   PR 7770 at commit d7df332.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

Test build #1310 has finished for   PR 7770 at commit d7df332.

This patch fails PySpark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

Test build #1313 has finished for   PR 7770 at commit f5b0d68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #1316 has finished for   PR 7770 at commit f5b0d68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Test build #39571 has finished for   PR 7770 at commit 9abecb9.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Merged build finished. Test PASSed.

Now that this has passed tests as of the latest commit, I'm going to merge it to master and branch-1.5.  Thanks!

Test build #1314 has finished for   PR 7770 at commit f5b0d68.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

Hey Patrick,
that sounds good.  I agree that there are probably a couple different concerns here.  My biggest concern was some overall direction we head on metrics ... as we've said we merging this is "safe" in the sense that it does let us change things later on without being tied to anything.  Sorry if I sounded snarky -- just expressing my objection to this direction without making a big fuss if its too late to do anything about it.

Other concerns aside ... can somebody explain the logic for resetting the accumulator values to me?
IIUC you reset the values:
a) on the initial instance
b) if the RDD in the dependency has entirely been evicted from the cache
c) if a stage fails prior to any task completions (but any remaining tasks from that stage attempt may still increment accumulators, I'm not certain)
d) if a stage fails in a way that we believe (in one go) that all map output data has been lost
you do not reset the values if:
e) only some of parent RDD has been evicted from the cache
f) if the stage fails after we've already got some map output registered
g) a stage has lost all of its map output, but spark doesn't "realize" this until after a few rounds of stage attempts (which is not that crazy at all -- perhaps even more likely than (d), though I am saying this anecdotally)
h) a stage has lost 99% of its map output data, but there is still one partition hanging on for dear life.
Is my understanding wrong?  Is this all intended behavior?  or "good enough" behavior?


Works on ubuntu64

Tested it on Ubuntu, it was failing. This fixes it.

I'm a bit late, but thanks.