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

[Bug]: Exporting a file with missing required attributes does not throw error. #2038

Open
3 tasks done
ehennestad opened this issue Feb 11, 2025 · 2 comments
Open
3 tasks done
Labels
category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users

Comments

@ehennestad
Copy link

ehennestad commented Feb 11, 2025

What happened?

The following code snippet throws a warning, but allows the user to export a file that breaks relative to the schema. According to the schema, the source_script_file_name property is required when source_script is filled out.

See here for an a small discussion in MatNWB: NeurodataWithoutBorders/matnwb#667

from pynwb import NWBHDF5IO, NWBFile, TimeSeries
from datetime import datetime
from dateutil import tz

session_start_time = datetime(2018, 4, 25, 2, 30, 3, tzinfo=tz.gettz("US/Pacific"))

nwbfile = NWBFile(
    session_description="Mouse exploring an open field",  # required
    identifier="test",  # required
    session_start_time=session_start_time,  # required
    source_script="create_nwbfile.py",  # optional
)

io = NWBHDF5IO("temp.nwb", mode="w")
io.write(nwbfile)
io.close()

Question: Should this throw an error instead?

Steps to Reproduce

See above

Traceback

Operating System

macOS

Python Executable

Python

Python Version

3.11

Package Versions

No response

Code of Conduct

@stephprince
Copy link
Contributor

I agree that according to the current schema, the example code you shared should throw an error instead of a warning to avoid the creation of invalid files, since file_name is a required attribute of source_script.

However I am also wondering if file_name should be made to be an optional attribute. The definition in the schema is a little unclear to me, it seems source_script could be the name of the file or a link to the source code (See also: #1451 (comment)). I think in either case the file_name attribute may not always be necessary.

@bendichter @rly do you know how these fields are being used and if file_name should be required?

@stephprince stephprince added category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users labels Feb 12, 2025
@rly
Copy link
Contributor

rly commented Feb 13, 2025

I agree with Ben's comment in #1451. I think source_script should be a link to a commit/tag/release in a public repo or a name of the package with a version string, and source_script_file_name should be the entry point / the file that was run within that repo. Now that we have was_generated_by which allows for multiple names and versions/identifiers, does it make sense to keep source_script and source_script_file_name?

Regardless, I think it would be OK to have source_script_file_name be optional. Most conversion packages have only one entry point, and it seems unnecessary to have to specify that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior priority: medium non-critical problem and/or affecting only a small set of NWB users
Projects
None yet
Development

No branches or pull requests

3 participants