ci: don't check invalid examples #90

Closed

adina wants to merge 2 commits from make-invalid into main

pull from: make-invalid

merge into: inm7:main

inm7:main

inm7:contr

inm7:grant

inm7:contribute

inm7:make-invalid-wildcard

inm7:author

inm7:contrib

inm7:readme

inm7:test-file-upload

inm7:short-link

inm7:studyact

inm7:reorder

Conversation 5 Commits 2 Files changed 1 +1 -1

adina commented 2025-08-13 06:41:21 +00:00

Owner

Copy link

there are no configurations for invalid examples and this - so far - only causes clutter. Can be easily commented back in when the necessary configurations are in place.

there are no configurations for invalid examples and this - so far - only causes clutter. Can be easily commented back in when the necessary configurations are in place.

adina added 1 commit 2025-08-13 06:41:22 +00:00

ci: don't check invalid examples ... b0ac6944cf

there are no configurations for invalid examples and this
only causes clutter

msz commented 2025-08-13 10:55:02 +00:00

Owner

Copy link

Thanks for tackling this, I agree the invalidation-related errors cause clutter.

How about a more surgical intervention? I need to check on more examples, but I believe this should work:

@@ -106,12 +106,12 @@ check-validation: \
 checkvalidation/%:
 	$(MAKE) checkvalid/$* checkinvalid/$*
 checkvalid/%: src/%/validation src/%.yaml
-	@for ex in $</*.valid.cfg.yaml; do \
+	@for ex in $(wildcard $</*.valid.cfg.yaml); do \
 		echo "Validate $$ex" ; \
 		linkml-validate --config "$$ex" || exit 5 ; \
 	done
 checkinvalid/%: src/%/validation src/%.yaml
-	@for ex in $</*.invalid.cfg.yaml; do \
+	@for ex in $(wildcard $</*.invalid.cfg.yaml); do \
 		echo "(In)validate $$ex" ; \
 		linkml-validate --config "$$ex" && exit 5 || true; \
 	done

The reason we see errors with the current makefile is that when there are no *.invalid.cfg.yaml files, we still make it into the for loop, and call linkml-validate with the glob expression verbatim, which produces errors that we see: "Invalid value for '--config': File 'src/flat-data/unreleased/validation/*.invalid.cfg.yaml' does not exist."

Brief search suggests that wildcard would avoid that behavior.

I am also not sure about the && exit 5 || true part - I must think about it.

Thanks for tackling this, I agree the invalidation-related errors cause clutter. How about a more surgical intervention? I need to check on more examples, but I believe this should work: ```diff @@ -106,12 +106,12 @@ check-validation: \ checkvalidation/%: $(MAKE) checkvalid/$* checkinvalid/$* checkvalid/%: src/%/validation src/%.yaml - @for ex in $</*.valid.cfg.yaml; do \ + @for ex in $(wildcard $</*.valid.cfg.yaml); do \ echo "Validate $$ex" ; \ linkml-validate --config "$$ex" || exit 5 ; \ done checkinvalid/%: src/%/validation src/%.yaml - @for ex in $</*.invalid.cfg.yaml; do \ + @for ex in $(wildcard $</*.invalid.cfg.yaml); do \ echo "(In)validate $$ex" ; \ linkml-validate --config "$$ex" && exit 5 || true; \ done ``` The reason we see errors with the current makefile is that when there are no `*.invalid.cfg.yaml` files, we still make it into the for loop, and call `linkml-validate` with the glob expression verbatim, which produces errors that we see: "Invalid value for '--config': File 'src/flat-data/unreleased/validation/*.invalid.cfg.yaml' does not exist." Brief search suggests that [wildcard](https://www.gnu.org/software/make/manual/html_node/Wildcard-Function.html) would avoid that behavior. ~I am also not sure about the `&& exit 5 || true` part - I must think about it.~

msz referenced this pull request from a commit 2025-08-13 11:56:05 +00:00

Stop makefile from calling linkml validate with config absent

msz referenced this pull request 2025-08-13 12:00:27 +00:00

Stop makefile from calling linkml validate with nonexistent config files #91

adina commented 2025-08-13 12:07:26 +00:00

Author

Owner

Copy link

TIL! Thanks for finding this. I tried it locally, and I agree it seems to work. I don't cling too tightly on the commit I made, but I made a companion PR in datalad-concepts which already got merged. If we use your method here, we should also match this in datalad-concepts.

TIL! Thanks for finding this. I tried it locally, and I agree it seems to work. I don't cling too tightly on the commit I made, but I made a companion PR in datalad-concepts which already got merged. If we use your method here, we should also match this in datalad-concepts.

adina added 1 commit 2025-08-13 12:14:09 +00:00

Use wildcard for matching configuration files bc11582967

adina commented 2025-08-13 12:16:42 +00:00

Author

Owner

Copy link

Ah, sorry for the commit. I realized too late you already opened #91.

Ah, sorry for the commit. I realized too late you already opened #91.

msz commented 2025-08-13 13:04:54 +00:00

Owner

Copy link

I don't cling to any particular commit either, but if we use the wildcard function then I would do it for both *valid and *invalid.

And I am not trying to compete or anything, it's just that I was also getting annoyed by the errors, so when I saw this PR it motivated me to look closer for a more specific solution. I was also surprised that @for withe the glob pattern behaves the way it does, and I think wildcard function is the more "do what we mean" approach.

I keep a closer eye on the Psychoinformatics hub than on GitHub at the moment, so I didn't realize until now that there is a companion PR in the GH datalad-concepts repo (which probably would be the more authoritative place, right?).

As a side note, probably out of scope here: it is weird that CI here failed in 17 min 34 s while the CI in functionally equivalent #91 succeeded in 2 min 14 s. Could it be possible that we are running into time limits or rate limiting? The CI output contains "Switch to local imports" and we've seen it rewrite some import statements, but probably we are still making external requests to original schemas - in GitHub?

I don't cling to any particular commit either, but if we use the wildcard function then I would do it for both `*valid` and `*invalid`. And I am not trying to compete or anything, it's just that I was also getting annoyed by the errors, so when I saw this PR it motivated me to look closer for a more specific solution. I was also surprised that `@for` withe the glob pattern behaves the way it does, and I think wildcard function is the more "do what we mean" approach. I keep a closer eye on the Psychoinformatics hub than on GitHub at the moment, so I didn't realize until now that there is a companion PR in the GH datalad-concepts repo (which probably would be the more authoritative place, right?). As a side note, probably out of scope here: it is weird that CI here failed in 17 min 34 s while the CI in functionally equivalent #91 succeeded in 2 min 14 s. Could it be possible that we are running into time limits or rate limiting? The CI output contains "Switch to local imports" and we've seen it rewrite some import statements, but probably we are still making external requests to original schemas - in GitHub?

adina commented 2025-08-13 13:27:05 +00:00

Author

Owner

Copy link

As a side note, probably out of scope here: it is weird that CI here failed in 17 min 34 s while the CI in functionally equivalent #91 succeeded in 2 min 14 s. Could it be possible that we are running into time limits or rate limiting? The CI output contains "Switch to local imports" and we've seen it rewrite some import statements, but probably we are still making external requests to original schemas - in GitHub?

I don't know. Out of curiosity, I restarted the run to see if this was maybe just a one time something. Afterwards, I'll close this PR as superseeded by #91. :) Edit: The rerun was 2mins15s. To be fair, that doesn't tell us anything. Let's keep an eye on the runs if we spot more long CIs.

> As a side note, probably out of scope here: it is weird that CI here failed in 17 min 34 s while the CI in functionally equivalent #91 succeeded in 2 min 14 s. Could it be possible that we are running into time limits or rate limiting? The CI output contains "Switch to local imports" and we've seen it rewrite some import statements, but probably we are still making external requests to original schemas - in GitHub? I don't know. Out of curiosity, I restarted the run to see if this was maybe just a one time something. Afterwards, I'll close this PR as superseeded by #91. :) Edit: The rerun was 2mins15s. To be fair, that doesn't tell us anything. Let's keep an eye on the runs if we spot more long CIs.

👍 1

adina closed this pull request 2025-08-13 13:31:35 +00:00

All checks were successful

Hide all checks

Codespell / Check for spelling errors (pull_request) Successful in 20s

Details

Model checks / lint (pull_request) Successful in 1m40s

Details

Validate examples and verify unmodified conversion / lint (pull_request) Successful in 2m37s

Details

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

wontfix

This will not be worked on

No labels

bug

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

inm7/inm7-concepts!90

No description provided.