Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FERC XBRL to SQLite data extractor #21261

Merged

Conversation

zschira
Copy link
Contributor

@zschira zschira commented Nov 24, 2022

Checklist

  • Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • License file is packaged (see here for an example).
  • Source is from official source.
  • Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • If static libraries are linked in, the license of the static library is packaged.
  • Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • Build number is 0.
  • A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • When in trouble, please check our knowledge base documentation before pinging a team.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/catalystcoop.arelle-mirror:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror) and found it was in an excellent condition.

@zschira
Copy link
Contributor Author

zschira commented Nov 24, 2022

I am willing to be listed as maintainer

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror, recipes/catalystcoop.ferc-xbrl-extractor) and found some lint.

Here's what I've got...

For recipes/catalystcoop.arelle-mirror:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: python.

@zschira zschira changed the title Add Catalyst Cooperative mirror of Arelle XBRL library Add FERC XBRL to SQLite data extractor Nov 24, 2022
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror, recipes/catalystcoop.ferc-xbrl-extractor) and found some lint.

Here's what I've got...

For recipes/catalystcoop.arelle-mirror:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

For recipes/catalystcoop.ferc-xbrl-extractor:

  • Non noarch packages should have python requirement without any version constraints.
  • Non noarch packages should have python requirement without any version constraints.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror, recipes/catalystcoop.ferc-xbrl-extractor) and found it was in an excellent condition.

recipes/catalystcoop.arelle-mirror/meta.yaml Outdated Show resolved Hide resolved
recipes/catalystcoop.arelle-mirror/meta.yaml Outdated Show resolved Hide resolved
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror, recipes/catalystcoop.ferc-xbrl-extractor) and found some lint.

Here's what I've got...

For recipes/catalystcoop.arelle-mirror:

  • noarch: python recipes are required to have a lower bound on the python version. Typically this means putting python >=3.6 in both host and run but you should check upstream for the package's Python compatibility.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/catalystcoop.arelle-mirror, recipes/catalystcoop.ferc-xbrl-extractor) and found it was in an excellent condition.

@zschira
Copy link
Contributor Author

zschira commented Nov 30, 2022

Hi @xhochy just wanted to check-in and see if these recipes are good now. We can try to remove the lxml build dependency if need be, however we are trying to make as minimal changes to the Arelle library as possible.

Thanks!

@zaneselvans
Copy link
Contributor

@xhochy Do you have any additional pointers on what we need to do to get this merged in?

@zschira
Copy link
Contributor Author

zschira commented Dec 5, 2022

@conda-forge-admin, please ping team

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@zaneselvans
Copy link
Contributor

Things I could imagine being the issue with getting this merged...

The lxml dependence in the build environment. Looking at the current Arelle branch, it seems like this is not longer present, which seems more normal. So maybe we can see if everything works without it?

The Windows builds are failing. I've previously had noarch packages on here where the Windows builds failed and there was some known problem that made that the expected behavior and it wasn't a problem (See #12902 and #18726) but I don't know if that's what's going on here. Isn't there some actual code compilation happening inside Arelle? It seems like it's intended to work on Windows from the setup.py contents. But we also don't have a Windows machine to debug on, so getting this working could be hard, and at least for our purposes, there are other parts of the PUDL project that already don't work on Windows.

We're packaging our fork of Arelle and not the official Arelle. This is more complex. Early this year when we started using Arelle it had fallen into disrepair, and wasn't even available via PyPI. We needed to make some very minor changes to deal with a locale bug (and maybe others? @zschira what changes other than packaging for release on PyPI did we end up making?). Since then it's been taken over by some company, and they've started doing some maintenance on the library, including modern packaging and releases on PyPI. Ultimately of course we would prefer to use the official Arelle release (assuming we can get our minor bugfixes integrated) but we are currently trying to do our own very delayed release, and are apprehensive about swapping out this low level dependency immediately beforehand. If this is a blocking concern that will prevent the package from getting released on conda-forge we could look at trying to switch over to the main Arelle repo, but right now we don't know how much work it would be.

Any direction y'all can provide would be much appreciated. This is the last thing preventing us from doing our own release and we'd really like to get it cleared up! Thank you for all your hard work!

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/arelle-release, recipes/catalystcoop.ferc-xbrl-extractor) and found it was in an excellent condition.

@zschira
Copy link
Contributor Author

zschira commented Dec 9, 2022

We've moved to using the official version of Arelle on PyPI. This new version does not have the lxml build dependency anymore. Please let me know if everything else looks good.

@conda-forge-admin, please ping team

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-webservice.

I was asked to ping @conda-forge/staged-recipes and so here I am doing that.

@zaneselvans
Copy link
Contributor

It does seem like there's some platform-specific compilation going on in here, at least for Windows, with DLLs and binary outputs (maybe for the GUI or CLI tools they bundle alongside the python library?), so it seems a little weird that it would be noarch @xhochy, but I really don't understand how all the multi-platform build specification stuff works.

So it seems like we need to do one of the following:

  • Determine that the failed Windows builds are spurious and ignore them, keeping the package noarch
  • Explicitly state that at least some or all of the conda package (like the GUI) are not expected to work on Windows.
  • Fix the failed Windows builds, which I have no idea how to do.

Looking at the end of the build outputs it seems like it might just be the case that it's failing because it generates a binary output (successfully?) but realizes that the package has been marked noarch and sees this as incompatible with having generated a binary output, and so fails.

  Building wheel for arelle-release (pyproject.toml): finished with status 'done'
  Created wheel for arelle-release: filename=arelle_release-2.2.3-py3-none-any.whl size=7632808 sha256=72df0374e8da09c0916203008f545ee3b278bc319491efc5f48c3daa0624401c
  Stored in directory: C:\Users\VssAdministrator\AppData\Local\Temp\pip-ephem-wheel-cache-kqo2e8yy\wheels\83\84\e8\b79538a26b8f10a7500b3d41b861bb01d0e10329de5929104c
Successfully built arelle-release
Installing collected packages: arelle-release

Successfully installed arelle-release-2.2.3
Removed build tracker: 'C:\\Users\\VssAdministrator\\AppData\\Local\\Temp\\pip-build-tracker-oaap55iw'

Resource usage statistics from building arelle-release:
   Process count: 4
   CPU time: Sys=0:00:03.4, User=0:00:11.6
   Memory: 106.1M
   Disk usage: 91.1M
   Time elapsed: 0:00:21.3


INFO:conda_build.build:Packaging arelle-release
Packaging arelle-release
INFO:conda_build.build:Packaging arelle-release-2.2.3-pyhd8ed1ab_0
Packaging arelle-release-2.2.3-pyhd8ed1ab_0
number of files: 520
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
Unknown format
   INFO: sysroot: 'C:/Windows/' files: '['winhlp32.exe', 'win.ini', 'twain_32/wiatwain.ds', 'twain_32.dll']'
   INFO (arelle-release,Scripts/arelleCmdLine.exe): Needed DSO C:/Windows/System32/KERNEL32.dll found in $SYSROOT
   INFO (arelle-release,Scripts/arelleCmdLine.exe): Needed DSO C:/Windows/System32/SHLWAPI.dll found in $SYSROOT
   INFO (arelle-release,Scripts/arelleGUI.exe): Needed DSO C:/Windows/System32/KERNEL32.dll found in $SYSROOT
   INFO (arelle-release,Scripts/arelleGUI.exe): Needed DSO C:/Windows/System32/USER32.dll found in $SYSROOT
   INFO (arelle-release,Scripts/arelleGUI.exe): Needed DSO C:/Windows/System32/SHLWAPI.dll found in $SYSROOT
   INFO (arelle-release): Interpreted package 'arelle-release' is interpreted by 'python'
[noarch_python] Noarch package contains binary script: arelleCmdLine.exe

@zaneselvans
Copy link
Contributor

@sagesmith-wf Do you happen to have any insight into what might be going on here with the Windows builds?

@sagesmith-wf
Copy link

@zaneselvans, I am unfamiliar with this error or the tooling, but the error seems indicative or an architecture mismatch. I would double check that you are using a 64bit python on 64bit windows because the error you posted is looking for system32 which seems wrong since the staged recipe says win_64.

@zaneselvans
Copy link
Contributor

zaneselvans commented Dec 12, 2022

@zschira Can we just disclaim windows compatibility for the moment, since there are clearly some components that won't work without additional effort? I imagine the Python library portion itself will work fine and it's just the GUI + CLI tools that are running into OS-specific build issues.

I.e. change the build section to:

build:
  skip: true # [win]
  script: {{ PYTHON }} -m pip install . -vv
  number: 1

@beckermr beckermr merged commit a3ec6ec into conda-forge:main Dec 12, 2022
@zaneselvans
Copy link
Contributor

❤️ Thank you!

@ryanvolz
Copy link
Contributor

I suspect you need some entry_points to make conda-build happy with installing this as noarch on Windows. Should be an easy fix in the feedstock.

build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv
number: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps add:

build:
  # ... what's already there
  entry_points:
    - arelleCmdLine = arelle.CntlrCmdLine:main
    - arelleGUI = arelle.CntlrWinMain:main

Copy link
Member

Choose a reason for hiding this comment

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

- pip

about:
home: https://pypi.org/project/arelle-release/
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps also add dev_url: https://github.com/Arelle/Arelle

Copy link
Member

@jakirkham jakirkham Dec 13, 2022

Choose a reason for hiding this comment

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

- pip

about:
home: https://github.com/catalyst-cooperative/ferc-xbrl-extract
Copy link
Contributor

Choose a reason for hiding this comment

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

this appears to actually be:

https://github.com/catalyst-cooperative/ferc-xbrl-extractor  # not "extract"

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, yes, will fix this.

about:
home: https://github.com/catalyst-cooperative/ferc-xbrl-extract
summary: A tool for extracting data from FERC XBRL Filings.
dev_url: https://github.com/catalyst-cooperative/ferc-xbrl-extract
Copy link
Contributor

Choose a reason for hiding this comment

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

if the home is already github, this doesn't add much

@jakirkham
Copy link
Member

Maybe we can turn these into issues on the respective feedstocks? Filed a couple, but please feel free to file more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants