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

Generating new schema for v1.9.0 #83

Merged
merged 9 commits into from
Oct 29, 2024
Merged

Conversation

Jeenashaji
Copy link
Contributor

@Jeenashaji Jeenashaji commented Sep 9, 2024

New schema_v1.9.0 is run with generatemodel.py and the new output file model_v1.9.0.py has been generated . Changes has been compared .

falu2421 added 2 commits September 6, 2024 13:02
…in the github workflow whenever a new schema is introduced in the schema folder
@erdalkaraca erdalkaraca changed the title Generating new schema for v9 Generating new schema for v1.9.0 Sep 11, 2024

- name: Run generatemodel.py
run: |
python ancpbids/generatemodel.py --schema-file ${{ steps.get_version.outputs.schema_file }} --version-tag ${{ steps.get_version.outputs.version_tag }}
Copy link
Collaborator

@erdalkaraca erdalkaraca Sep 11, 2024

Choose a reason for hiding this comment

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

it seems you added support for parameters but the generatemodel.py has no changes in this commit: please make sure you also commit the changes to generatemodel.py... please also make sure that the path to the generatemodel.py is correct, i.e. should be ancpbids/tools/generatemodel.py

on:
push:
paths:
- 'schema/*.json' # Triggers only when a new schema JSON file is added or modified
Copy link
Collaborator

Choose a reason for hiding this comment

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

so, an ancpBIDS dev would have to copy a new schema.json from the BIDS spec repo over to ancpBIDS, push to main, then this action/workflow will be triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, since i copied the new schema.json file to the schema and ran it, i thought that can be trigger point.


- name: Update version_tag in generatemodel.py
run: |
sed -i "s/version_tag = 'v[0-9]\+\.[0-9]\+\.[0-9]\+'/version_tag = '${{ steps.get_version.outputs.version_tag }}'/" ancpbids/generatemodel.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

you already parameterized the script, just use the parameter --version-tag in the script instead of changing it inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified the code just to use the version_tag. But i saw that the workflow still failed in some tests. Should i push the changes or would you like to review it once?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, please push the changes to your pul lrequest

@erdalkaraca
Copy link
Collaborator

after thinking a bit more about the process, it might be better to integrate the automation directly into the generatemode.py script:
$ python generatemodel.py --version_tag=1.9.0

This would download the schema.json file from the appropriate branch in the BIDS repo and save it as schema-{version_tag}.json in the schema/ folder. Then call the existing code to generate a new python model for the downloaded schema.
Any thoughts?

falu2421 and others added 2 commits October 1, 2024 16:48


# Function to fetch the latest schema version from GitHub
def fetch_latest_schema_version():
Copy link
Collaborator

Choose a reason for hiding this comment

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

it may be better to parameterize this function to allow a specific version to be generated and only generate the latest version if the parameter is not set:

def fetch_schema_version(version_tag=None): ...

Copy link
Collaborator

@erdalkaraca erdalkaraca Oct 1, 2024

Choose a reason for hiding this comment

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

and also make sure, the user provided a version_tag >= 1.8.0 as the schema.json was only introduced beginning with 1.8.0

module_version_tag = version_tag.replace('.', '_')
generator = ClassGenerator("../schema/model_base.yaml", f"../schema/schema_{version_tag}.json", module_version_tag)
generator.generate(version_tag)
# Fetch the latest schema version and URL
Copy link
Collaborator

Choose a reason for hiding this comment

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

re-introduce arg parser to allow providing a specific schema version to be generated

@erdalkaraca
Copy link
Collaborator

So far, I like your approach to download the schema.json!

When running the test suite, two tests regarding the schema loading fail: please try to understand why they are failing and maybe come up with a fix.

falu2421 and others added 5 commits October 29, 2024 00:20
…schema_version(): to allow a specific version to be generated and only generate the latest version if the parameter is not set , Changed test_schema.py script to accomodate new version v1.9.0
@erdalkaraca erdalkaraca merged commit e8cc161 into ANCPLabOldenburg:main Oct 29, 2024
1 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants