Add support for import from CSV and import dir #253
Conversation
Perun import now supports specification of imported profiles through CSV files and allows to specify the import directory. CLI and CSV interface were extended with the ability to specify custom profile stats.
JiriPavela
force-pushed
the
feature-generic-profile-stats
branch
from
9be4d5e to
72c9237
Compare
There was a problem hiding this comment.
If you need it merge, feel free to merge it, but address my issues in separate PR.
The fixed median aggregation is blocker for me, but it does not need to be fixed in this PR. The rest are minor stuff. If you need consultation regarding this or short call, you can ping me on discord (but after 4pm).
| "--exitcode", | ||
| "-e", | ||
| "--stats-info", | ||
| "-t", |
| default="?", | ||
| help=("Exit code of the command."), | ||
| default=None, | ||
| metavar="<stat1-description,...>", |
There was a problem hiding this comment.
| metavar="<stat1-description,...>", | |
| metavar="[STAT]", |
In my WIP I'm rewritting the metavars to conform to [...] notation. Please, use this.
| help=("Exit code of the command."), | ||
| default=None, | ||
| metavar="<stat1-description,...>", | ||
| help="Describes the stats associated with the imported profiles. Please see the import " |
There was a problem hiding this comment.
Shouldn't this be file? If not, change to [STR] and write an example, how, the stats should look.
| @dataclass | ||
| class ProfileStat: | ||
| ALLOWED_ORDERING: ClassVar[dict[str, bool]] = { | ||
| "higher_is_better": True, |
There was a problem hiding this comment.
I would consider simplifying to better with type Literal["higher", "lower"]. I could imagine, that there might be case, when there is something different (maybe equal?).
| unit: str = "#" | ||
| ordering: bool = True | ||
| tooltip: str = "" | ||
| value: int | float = 0.0 |
There was a problem hiding this comment.
float subsumes int, I would keep float.
|
|
||
| # Third-Party Imports | ||
| import gzip | ||
|
|
||
| # Perun Imports | ||
| from perun.collect.kperf import parser | ||
| from perun.profile import helpers as profile_helpers | ||
| from perun.profile import helpers as p_helpers |
There was a problem hiding this comment.
If possible, please, don't rename file "owner's" notations. I don't see the saving here.
| @@ -66,12 +165,13 @@ def import_from_string( | |||
| ) | |||
| prof.update({"origin": minor_version.checksum}) | |||
| prof.update({"machine": get_machine_info(machine_info)}) | |||
| prof.update({"stats": [asdict(stat) for stat in profiles.aggregate_stats(statistics.median)]}), | |||
There was a problem hiding this comment.
This definitely should not be fixed to median, there is helper function that does the aggregation and supports more function and is set wrt configure option. Please, rewrite this: the agg parameter should preferably be completely removed and the helper function should be used instead. See https://github.com/Perfexionists/perun/blob/devel/perun/utils/common/common_kit.py#L628 and
Line 697 in c2c10cc
|
Great, thanks for the review, I'll merge it and release it and resolve the issues in a subsequent PR. As for the issue with the fixed median: this is just a temporary solution that we agreed on with Jirka H., so that the CSV import feature is not blocked until a proper (already thoroughly discussed) solution is implemented in the future. |
Yeah, I know it is hotfix/workaround, I just wanted to stress that I find this important to be fixed in near future while you are in the context. |
Perun import now supports specification of imported profiles through CSV files and allows specifying the import directory.
CLI and CSV interface were extended with the ability to specify custom profile stats.
Resolves #250
Resolves #251