Add EMBER support for automatic upload

[CodyCBakerPhD]

Necessary along the path to NeurodataWithoutBorders/nwb-guide#989

Results can be seen at https://dandi.emberarchive.org/dandiset/000431

Might swap this to a staging Dandiset once that location stabilizes a bit (still a little in the air)

[h-mayorquin]

There are some conflicts at the moment.

[h-mayorquin]

This looks good to me, sorry for taking so long to review. Some things to decide about the testing infra but should be good to go.

[h-mayorquin]

why are you removing this?

[CodyCBakerPhD]

It's moved up to https://github.com/catalystneuro/neuroconv/pull/1486/files#diff-043deaacc59c256b39a4c09f0fc74d19afcbae754774bb703a95216582b77171R72 dependent on instance value

May actually prefer introducing a token input kwarg so a user can decide which token to use (rather than requiring an environmental variable of a particular, possibly misleading name)

[h-mayorquin]

instance_token?

[CodyCBakerPhD]

Sure. You want to do that here or in separate PR

[h-mayorquin]

Here is OK if you are Ok with it.

[CodyCBakerPhD]

Actually upon looking deeper, the core method for upload on DANDI side does not allow specific client to be passed: https://github.com/dandi/dandi-cli/blob/master/dandi/upload.py#L123

And instead autogenerates a client and authenticates it through the required environment variable (again cannot be passed explicitly): https://github.com/dandi/dandi-cli/blob/master/dandi/dandiapi.py#L500

If this changes we can come back to this but for now nothing to do

[h-mayorquin]

Uh, i see, Ok.

[h-mayorquin]

Damn, the ruamel thing : /

https://sourceforge.net/p/ruamel-yaml-clib/tickets/47/

Seems the made a bad release, want to constrain the pyproject with this version as a ceiling temporary?

[CodyCBakerPhD]

Seems the made a bad release, want to constrain the pyproject with this version as a ceiling temporary?

It has been blacklisted. Ran into it independently on nwb2bids as well

[h-mayorquin]

Apparently they released a fix now:

https://sourceforge.net/p/ruamel-yaml-clib/tickets/47/

But I see a key issue. Does it sound familiar to you or should I look into the CI to see if there is a secret missing?

[CodyCBakerPhD]

But I see a key issue. Does it sound familiar to you or should I look into the CI to see if there is a secret missing?

The current test failure in the CI does not seem to reference any lines of code that I have changed here

[h-mayorquin]

Ok, I will take a look, on the other hand the tests on other PRs are not failing so I wonder:

#1513

[h-mayorquin]

@CodyCBakerPhD

So I checked this today and the reason the tests are failing is because this comes from a fork. To merge this, I will fork this so it can come from a branch as I don't feel like an overhaul of how do we handle the secrets with branches should block this and this has been opened for a while.

See:
#1519

[CodyCBakerPhD]

So I checked this today and the reason the tests are failing is because this comes from a fork.

Oh, right. Forgot about that

To merge this, I will fork this so it can come from a branch as I don't feel like an overhaul of how do we handle the secrets with branches should block this and this has been opened for a while.

Actually if you just change the base to a fresh branch off of main, merge into that, then re-open yourself from that branch to main, it should be cleaner than you forking my fork

Otherwise that sounds like a good approach in general for staging/testing contributions from forks (as a 2-step process)

[h-mayorquin]

Otherwise that sounds like a good approach in general for staging/testing contributions from forks (as a 2-step process)

Yes, I also realized that because of the skipif the tests were being skipped form a fork. That is, we had the danger of a false positive.

1. Fork submit changes to live-services
2. Reviewer does not look carefully that tests were skipped and sees green light
3. Approves and merges faulty code to main.

So I removed the skip ifs and will make the error earlier and more informative.

[CodyCBakerPhD]

@h-mayorquin OK I updated this branch with the updated testing suite split (and workflow adjustments)

Please update your other testing setup accordingly

[codecov]

Codecov Report

❌ Patch coverage is 5.00000% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.79%. Comparing base (3d21242) to head (1caeae1).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/neuroconv/tools/data_transfers/_dandi.py	5.00%	19 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1486      +/-   ##
==========================================
- Coverage   89.90%   89.79%   -0.11%     
==========================================
  Files         155      156       +1     
  Lines       10016    10065      +49     
==========================================
+ Hits         9005     9038      +33     
- Misses       1011     1027      +16     

Flag	Coverage Δ
unittests	89.79% <5.00%> (-0.11%)	⬇️

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

Files with missing lines	Coverage Δ
src/neuroconv/tools/data_transfers/_dandi.py	13.84% <5.00%> (-2.83%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.