search

calgo-lab / green-db

The monorepo that powers the GreenDB.
https://calgo-lab.github.io/green-db/
22 stars 2 forks source link

Add the UNAVAILABLE certificate. #115

Closed itrajanovska closed 1 year ago

BigDatalex commented 1 year ago

Hey, I think it would be great to have some description of all PRs, even if it's just a small one. So, in this case, I think it is kinda important to mention that this change affects the ecosia-exporter repo (see here), because the poetry environment of the exporter references this branch.

We have talked about it in the morning, but honestly, I don't feel comfortable with approving this. As an example, we have also added other (regular) sustainability labels and these wouldn't be part of this branch. In other words, what speaks against merging the main branch into this one?

Maybe @se-jaeger can have a look at this and say if there might be any implications.

itrajanovska commented 1 year ago

Hi,

I didn't add any documentation as I didn't know that that was a requirement for every MR. But for this small of a change I thought making this MR was a step enough to make everyone aware that we need to update the special labels file - and as added in the json change itself there is a description in German already of why that label is needed.

"description": "Dieses Label wird für Produkte verwendet, die kein Nachhaltigkeits-Label haben, aber ein anderes, aus einem ML-Modell abgeleitetes Label tragen."

I think the new labels should be added in this (test-database-with-new-poetry) branch as well, as if I'm not mistaken, they won't pass the main::to_df method, as there are constraints in the Product class, specifically for the sustainability_labels in this case and which values are allowed to it. And, after checking the labels - core/*.json files, there wasn't any change by comparing them with the main branch.

Also, regarding why are we using that branch, I haven't seen any comment left on that anywhere, and I don't know if it's safe to merge the main branch in it, but when trying to switch to the main branch in the dependencies section once, I got the following message:

Because database (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=database depends on core (0.2.4) @ file:///Users/ivana/DataSpellProjects/entity-matcher/.venv/src/green-db/core
 and entity-matcher depends on core (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=core, database is forbidden.
So, because entity-matcher depends on database (0.2.4) @ git+https://github.com/calgo-lab/green-db@main#subdirectory=database, version solving failed.

... So, that's why I assumed there are some dependency issues, and that's why I continued adding my changes on that branch.

se-jaeger commented 1 year ago

I think it would be much nicer to fix this one #94 and use this repos' main branch for the ecosia-exporter as dependency. Then we don't need this branch anymore.

itrajanovska commented 1 year ago

I think it would be much nicer to fix this one #94 and use this repos' main branch for the ecosia-exporter as dependency. Then we don't need this branch anymore.

Thanks for the prompt reply @se-jaeger ! For the sake of shipping the EM model with next weeks data extraction I would still add this now as a quick fix. Even if it's not for the EM model only, the Ecosia export will fail as well because of the main::to_df issue that I mentioned above. I'd be happy to get back to this once I have more time and solve it properly. However for now - given the priorities and the fact that this branch existed for a longer period due to an unrelated long-running issue, I think this is a good quick fix.

itrajanovska commented 1 year ago

Thanks for the feedback in the DM Alex,

I also misunderstood your point in the first place. Now I just merged main into this branch accordingly and no further changes will be done on this branch unless we have them on main.